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

Can someone please take a look at the below script to review it for any flaws or suggestions that would make it a rock solid script. You will notice I put another find to compare against the original. Thanks, Dave
#!/usr/bin/perl -w #use strict; use File::Find; use File::stat; my @results; my $ext = "A"; find(\&search, "c:/test"); sub search { if ($_ =~ m/ACH/ ) { push @results, [$_, stat($_)->mtime, stat($_)->size]; } } @results = sort { $a->[1] <=> $b->[1]} @results; foreach my $x (@results){ rename ($x->[0], "USB" . $ext); print $x->[0] . "\t" . localtime($x->[1]) . $x->[0]->size ."\n"; ++$ext; } my @temp; find(\&post, "c:/test"); sub post { if ($_ =~ m/USB/ ) { push @temp, [$_, stat($_)->mtime]; } } foreach my $y (@temp) { print $y->[0] . "\t" . localtime($y->[1]) . "\n"; }

Replies are listed 'Best First'.
Re: Script Critique
by toolic (Bishop) on Jul 21, 2008 at 01:40 UTC
    suggestions that would make it a rock solid script
    Yeah, un-comment line #2:
    use strict;

    You could also add some documentation on what it accomplishes, under what conditions and limitations, preferably as POD.

    Have you created a test suite, using a CPAN module such as Test::More?

Re: Script Critique
by ikegami (Patriarch) on Jul 21, 2008 at 01:59 UTC
    • Why is use strict; commented?!

    • Your script only works when the current directory is the same as the one passed to find and when no matching files are found in subdirectories. Some of the instances of $_ should be $File::Find::name.

    • If you don't care to search subdirs, File::Glob's bsdglob might be more useful than File::Find.

    • Why do you do a second find? Either there weren't any USB file to begin with, in which case you already know all the USB files since you just created them, or you are clobbering some USB files while leaving others behind (which doesn't sound correct).

    • You're doing a case-sensitive match on a case-insensitive file system. The "i" modifier should be used on your match operators.

Re: Script Critique
by kyle (Abbot) on Jul 21, 2008 at 02:07 UTC

    Use Getopt::Long and Pod::Usage to take parameters and output help (I agree with toolic's suggestion to add documentation in POD—see perlpod).

    For @results and @temp, I say first of all rename them. Second, instead of storing array references, use hash references with beautifully named elements.

    push @results_renamed, { filename => $_, mtime => stat($_)->mtime(), bytes => stat($_)->size(), };

    Then your sort and a lot of other things become a lot more legible.

    @results_renamed = sort { $a->{mtime} <=> $b->{mtime} } @results_renamed;

    Check whether rename succeeded and die or warn with $! in the message.

    I think $x->[0]->size is a bug. If you'd left use strict in there, I think Perl would have told you so. So use strict!

    Say "scalar localtime" explicitly rather than counting on concatenation for the scalar context.

    It might be a good idea to be more specific about what files you want to operate on. There's nothing wrong with m/ACH/, but I suspect you mean /^ACH/ or something even more specific. If this is accidentally used somewhere it should not be, what happens?

    I prefer anonymous code references over references to named subs that aren't used for anything else.

    my @temp; my $post = sub { push @temp, { filename => $_, mtime => stat($_)->mtime } if /USB/ }; find( $post, 'c:/test' );

    This way $post is a lexical and does not run the risk of running up against another sub somewhere. You can take that code off into its own sub and be sure it doesn't interfere with anything else.

    sub find_usb { my @temp; my $post = sub { push @temp, { filename => $_, mtime => stat($_)->mtime } if /U +SB/ }; find( $post, 'c:/test' ); return @temp; }
      Thank you all for your feedback, its been helpful. I am a new bee at Perl, so I want try to improve hand over fist as I go and not be content with mediocre code. BTW - I normally use strict, so I'm not sure why it was greyed out. I probably frustrated with getting this to work. Thanks again, drod
Re: Script Critique
by GrandFather (Saint) on Jul 21, 2008 at 01:56 UTC

    Always use strictures (use strict; use warnings; - see The strictures, according to Seuss).

    Don't mix subs and main line code. Perl lets you, but it makes it vary hard to see the flow of the main line code.

    Where does @results get set? Nothing in the main line code indicates that. Setting global variables as a side effect of calling code is bad!

    Will all elements of @results be numeric? If not then your lack of strictures has set you up for nasty results. The spaceship operator <=> numifies its augments and most strings become 0. If you want a string compare use cmp.


    Perl is environmentally friendly - it saves trees

      Where does @results get set? Nothing in the main line code indicates that. Setting global variables as a side effect of calling code is bad!

      He doesn't control File::Find's interface. He has to use a global unless he wishes to mix his input, processing and output into the find callback, something I wouldn't do. However, inlining the function makes things appear more local.

      my @results; find( sub { ... push @results, ...; }, $dir );

      Update: That's one of the reasons I prefer File::Find::Rule.

Re: Script Critique
by leocharre (Priest) on Jul 23, 2008 at 01:55 UTC

    Something that you should consider later on..

    There are various philosophies of APIs, interfaces between not just a program and users, but the interface between the coder and what you are using.

    One possible philosophy is as follows.

    A script is not meant to be solid. A script is throw away. A script may be quite short, although it may set off a chain of complex events.

    One of the wonderful things of perl is that you can store complex code in modules. And scripts can make calls in various ways to library code (modules, etc).

    Per this kinf of ideology- The way to make your 'code' solid, would be to set up a test suite for your functions and procedures.
    After that, a script would merely take in input perhaps, check it, and perhaps spit output or feedback.

    Thus, seeking a 'solid' script could be considered to be- finding the perfect cup from which to drink beer. When perhaps the beer's source is more important.

    Again, this is just one way to look at it.
    I hope this sounds interesting and doesn't annoy too much.