zjunior has asked for the wisdom of the Perl Monks concerning the following question:

Hi Monks. Consider this slice of code:


# (...)
sub indexfiles {
    my $cod  = $_[0] || die $!;
    my $root = $_[1] || '.';
    # XXX how to correct this?
    our @files;

    find(\&wanted, $root);
    sub wanted 
    {
        push(@files, $_) if /\.mp3$/;
    }

    open(DB, "> $dbpath/$cod.txt") || die $!;
    foreach(sort @files)
    {
        s/\.mp3$//;
        print DB $_, "\n";
    }
    close(DB);
}
# (...)

[download]

In my habitual novice ignorance I tried to change the code a few times to look mode elegant. I don't like a sub inside another sub. And declaring a variable like that with our scares me, and possibly all you. :)

How to handle that with correctness, without mess the code and handle properly the @files array context?

Hoping that it wouldn't be too stupid... -- zjunior

Replies are listed 'Best First'.
Re: Using File::Find with a bit of elegance
by Kanji (Parson) on Mar 31, 2002 at 01:25 UTC

    Use an anonymous sub...

    my @files;
find( sub { push @files, $_ if /\.mp3\z/ } => $root );

    [download]

        --k.


[reply]
[d/l]
(jeffa) Re: Using File::Find with a bit of elegance
by jeffa (Bishop) on Mar 31, 2002 at 01:30 UTC
    Here is a 'straight script' version. Personally, for scripts this small, i really prefer this style (Perl Cookbook recipe 9.7.) 
    use strict;
use File::Find;

my $cod    = shift or die 'some error message';
my $dbpath = '/path/to/somewhere';
my @files;

@ARGV = '.' unless @ARGV;

find sub {
   push(@files, $_) if /\.mp3\z/;
}, @ARGV;

open(DB, ">$dbpath/$cod.txt") || die $!;

print DB map { s/\.mp3\z/\n/; $_ } sort @files;

    [download]
    The need for our has been eliminated because there are no subroutines to share variables across scope (which, IMHO, is a bad idea and an abuse of our).

    Looks like the original author was trying to encapsulate some functionality. I question this approach, because the subroutine indexfiles handles too many tasks and while it does accept arguments, indexfiles does not return anything and instead relies on a shared variable. (peruse the search results at google.com for tight coupling loose cohesion for some good reading.)

    Like i said before, i personally prefer a script like this to be consise and without unnecessary subroutines. But, since we are on the topic, here is another version that uses two subroutines (not including the File::Find magic): 

    use strict;
use File::Find;

my $cod    = shift or die 'some error message';
my $dbpath = '/path/to/somewhere';

@ARGV = '.' unless @ARGV;
my @files = get_files(@ARGV);
open(DB, ">$dbpath/$cod.txt") || die $!;

print DB trim(@files);

sub get_files {
   my @file;
   find sub {
      push(@file, $_) if /\.mp3\z/;
   }, @_;
   return @file;
}

sub trim {
   return map { s/\.mp3\z/\n/; $_ } sort @_;
}

    [download]
    Add some error checking and you have some nice subroutines. The important thing to remember here is that a subroutine should be as general as possible, only handle as small of task as possible, and act like a mathematical function: input and output.

    Good luck :)

    jeffa

    L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
B--B--B--B--B--B--B--B--
H---H---H---H---H---H---
(the triplet paradiddle with high-hat)
[reply]
[d/l]
[select]
      I preach about separation of presentation and content when i discuss CGI scripts, but i unintentionally strayed from that principle with this subroutine: 
      sub trim {
   return map { s/\.mp3\z/\n/; $_ } sort @_;
}

      [download]
      How? By 'decorating' each filename in the list with a newline. Oops!

      Better would be the following: 

      sub trim {
   return map { s/\.mp3\z//; $_ } sort @_;
}

# and decorate it in the 'client'
print DB map {"$_\n"} trim(@files);

      [download]
      Of course, the downside is two maps instead of one, but, this is better when making generic, robust, reusable subroutines. Allowing the client to make this decision could very well cut back on the number of modifications required for that subroutine.

      jeffa

      L-LL-L--L-LL-L--L-LL-L--
-R--R-RR-R--R-RR-R--R-RR
B--B--B--B--B--B--B--B--
H---H---H---H---H---H---
(the triplet paradiddle with high-hat)
[reply]
[d/l]
[select]
Re: Using File::Find with a bit of elegance
by runrig (Abbot) on Mar 31, 2002 at 04:44 UTC
    Including "$!" as part of that first error message is useless. "$!" is only for system error messages, like why you can't open a file and such. Also, as long as the purpose of your function is to write filenames, there's no point in saving them in an array (IMO):
    sub indexfiles {
  my $cod = shift || die "Must supply filename!";
  my @dirs = @_ ? @_ : ".";

  open(DB, ">$dbpath/$cod.txt") or die "Error: $!";
  my $fh = select(DB);
  local $\ = "\n";
  find( sub {
    local $_ = $_;
    print if s/\.mp3$//;
  }, @dirs);
  close DB;
  select($fh);
}

    [download]
    ----------
ooo  O\O  ooo tilly was here :,(
[reply]
[d/l]
Re: Using File::Find with a bit of elegance
by jepri (Parson) on Mar 31, 2002 at 01:42 UTC
    You can also just move the 'wanted' sub outside the 'indexfiles' sub and it should function exactly the same. However passing variables to it is tricky.

    ____________________
    Jeremy
    I didn't believe in evil until I dated it.

[reply]

Back to Seekers of Perl Wisdom