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

I have perl scripts I make for different tasks and I slowly add to them as needed based on additional requirements. Being a novice at perl scripting I know many of them are not as efficient as they probably could be.

I've linked one below, everything works, no errors or warnings but it's messy and it feels like I repeated a couple tasks for lack of a better idea of how to combine them.

Pasted below, any and all helpful suggestions are appreciated. I commented most of the stuff to sorta explain the point but feel free to say so if something is unclear.

use DirHandle (); use File::Spec (); use strict; use warnings; # Sets basic globals my $now = localtime; my $dir = "C:/Data"; my $dirL = "C:/DataL"; my $fCount=0; my $fCountL=0; # Sorts files in $dir and sets $latest->{file} with newest file. my $latest = (sort {$b->{mtime} <=> $a->{mtime}} map {{mtime => -M $_, file => $_}} <$dir/*>)[-1]; my $newM = (stat $latest->{file})[9]; # Sorts files in $dir and sets $oldest->{file} with oldest file. my $oldest = (sort {$a->{mtime} <=> $b->{mtime}} map {{mtime => -M $_, file => $_}} <$dir/*>)[-1]; my $oldM = (stat $oldest->{file})[9]; # Opens $dir and counts number of files opendir(DIR, $dir); LINE: while(my $FILE = readdir(DIR)) { next LINE if($FILE =~ /^\.\.?/); $fCount++; } closedir(DIR); # Opens $dirL and counts number of files opendir(DIRL, $dirL); LINE: while(my $FILE2 = readdir(DIRL)) { next LINE if($FILE2 =~ /^\.\.?/); $fCountL++; } closedir(DIRL); # Changes Newest/Oldest file display in email based on if any files ar +e found. my $latestF =''; my $oldestF =''; my $noFiles = 'Data: Empty directory, no files found that need to be p +rocessed.'; my $noFilesL = 'DataL: Empty directory, no files found that need to be + processed.'; if (($latest->{file}) || ($oldest->{file})){ $latestF = 'Newest File: '. $latest->{file}. ' with a timestamp of + '. scalar localtime $newM; $oldestF = 'Oldest File: '. $oldest->{file}. ' with a timestamp of + '. scalar localtime $oldM; $noFiles = ''; } exit;

Replies are listed 'Best First'.
Re: Looking for some assistance in cleaning up a perl script
by ww (Archbishop) on Jun 22, 2011 at 18:49 UTC
    If that script were 20-40 lines, posting it here (not as a NON-linking URL -- see Markup in the Monastery -- at pastebin) would be appropriate.

    Since it's 80-some lines, you'd do better to pick a section about which you're particularly concerned, to post (as a runnable snippet) here.

    We're here to help you learn; not to provide free programming services*1, which is fundamentally what you've asked for. See On asking for help and the other basic Q&A in PerlMonks FAQ.

    One direct comment: You start out using numeric values ($countf=0;) for example, but then proceed to use an alpha test, ge, on $countf at line 56, and revert to a numeric test in 59.

    Perl's DWIM factor will often save you from goofs like that... but you can't count on it forever.

    Update: Fixed some punct and quoted code.

    *1 ...or -- for the next 20 minutes only -- take advantage of the special offer of free programming services (except for a modest $2k registration fee, taxes, shipping, handling and modest surcharges) at ww.inc. :-)

    Update:

    OP revised since answers started appearing leaving several to appear out of contest. Author has been advised that doing so removes context for several replies (including this one).
    Original post included only a pastebin address for code, http://pastebin.com/P5mK7k5G (original was * NOT * a link) which was very different from what's now posted.

    In that code, lines 54-59 read:

    # Changes subject based on amount of files found, over 4000 will sugge +st a possible error. my $subject = ''; if ($fCount ge '4000') { $subject = '[POSSIBLE ERROR] Import has '.$fCount.' regular fi +les and '.$fCountL.' large files waiting to be processed!'; } elsif ($fCount < '4000') {

    Thanks to planetscape for pointing this out.

      We're here to help you learn; not to provide free programming services

      Hmm, we've done code reviews in the past, up to 1000 lines

Re: Looking for some assistance in cleaning up a perl script
by roboticus (Chancellor) on Jun 22, 2011 at 18:53 UTC

    shadowfox:

    First, if you use code tags, then the script will be easy to read. Second, this thread would become useless if/when that link dies--so please edit the node and insert your code to make the link more useful. This is a learning site, after all, and we encourage people to search the site for answers to their problems.

    ...roboticus

    When your only tool is a hammer, all problems look like your thumb.

Re: Looking for some assistance in cleaning up a perl script
by rev_1318 (Chaplain) on Jun 22, 2011 at 18:53 UTC

    No need to use pastbin. You can add your code here just fine using code-tags (and readmore-tags)...

    Some first comments:

    use variable names which mean something, also to others :)

    use consistent indentation in your code to improve readability

    use lexical file and directory handles

    this:

    # Sorts files in $dir and sets $latest->{file} with newest file. my $latest = (sort {$b->{mtime} <=> $a->{mtime}} map {{mtime => -M $_, file => $_}} <$dir/*>)[-1]; my $newM = (stat $latest->{file})[9]; # Sorts files in $dir and sets $oldest->{file} with oldest file. my $oldest = (sort {$a->{mtime} <=> $b->{mtime}} map {{mtime => -M $_, file => $_}} <$dir/*>)[-1]; my $oldM = (stat $oldest->{file})[9];
    to determine the oldest and newest file, could easier be writen as:
    my ($newM, $oldM) = (sort {-M $a <=> -M $b} <./*>)[0,-1];
    No need for maps etc. (oh, and better use glob...)

    HTH,

    Paul

      Thanks for the tip about the code tags, I ignorantly used [code][/code] to start with and it didn't work so I pastbined it here to avoid cluttering the front page. Good call on the readmore tags though, it made me realize the tag syntax I was using was the problem, I should have investigated it further.

      Anyway, I shorted it down to the part of concern, there probably isn't much that can be improved to the smtp email sending and logging so I took it out from the example. Thanks for the comments so far, I'll make some reflected changes tomorrow and report back with an update.

Re: Looking for some assistance in cleaning up a perl script
by jwkrahn (Abbot) on Jun 22, 2011 at 21:33 UTC
    # Sorts files in $dir and sets $latest->{file} with newest file. my $latest = (sort {$b->{mtime} <=> $a->{mtime}} map {{mtime => -M $_, file => $_}} <$dir/*>)[-1]; my $newM = (stat $latest->{file})[9]; # Sorts files in $dir and sets $oldest->{file} with oldest file. my $oldest = (sort {$a->{mtime} <=> $b->{mtime}} map {{mtime => -M $_, file => $_}} <$dir/*>)[-1]; my $oldM = (stat $oldest->{file})[9];

    Why sort the same data twice?

    # Sorts files in $dir and sets $latest->{file} with newest file and se +ts $oldest->{file} with oldest file. my ( $latest, $oldest ) = ( sort { $a->{ mtime } <=> $b->{ mtime } } map { { mtime => -M $_, file => $_ } } <$dir/*> )[ 0, -1 ]; my ( $newM, $oldM ) = map +( stat )[ 9 ], $latest->{ file }, $oldest-> +{ file };


    # Opens $dir and counts number of files opendir(DIR, $dir); LINE: while(my $FILE = readdir(DIR)) { next LINE if($FILE =~ /^\.\.?/); $fCount++; } closedir(DIR); # Opens $dirL and counts number of files opendir(DIRL, $dirL); LINE: while(my $FILE2 = readdir(DIRL)) { next LINE if($FILE2 =~ /^\.\.?/); $fCountL++; } closedir(DIRL);

    You should always verify that opendir worked correctly.    And, you don't include the directory name in your regular expression test so you are testing the wrong files and you can use grep to get your counts:

    # Opens $dir and counts number of files opendir my $DIR, $dir or die "Cannot opendir '$dir' because: $!"; my $fCount = grep "$dir/$_" !~ /\A\.\.?\z/, readdir $DIR; closedir $DIR; # Opens $dirL and counts number of files opendir my $DIRL, $dirL or die "Cannot opendir '$dirL' because: $!"; my $fCountL = grep "$dirL/$_" !~ /\A\.\.?\z/, readdir $DIRL; closedir $DIRL;
Re: Looking for some assistance in cleaning up a perl script
by toolic (Bishop) on Jun 22, 2011 at 23:26 UTC

      Hey sorry i got busy with other tasks, only just had a chance to get back on this. So I tried the grep suggestion but i had a problem with the count of files in the directory being 2 more than were actually there, even when its empty? I wasn't sure why this would happen, there are no hidden or system files or sub directries in this windows directory. So I tried another approach, as you can see below. Will grep save time and how can I manage the output to ignore the 2 'ghost' items its counting?

      Also I had to add in a bit to check for .lock files that are created when import jobs start, I was wondering if this can be consened also? I'm curious if anyone has some reading that pertains to what types of things can be run in the same "query" for lack of a better term, cause I end up doing things seperately for each occourance I need for lack of understanding.

      # Looks in $dir and sets $latest->{file} with newest file and sets $ol +dest->{file} with oldest file. my ( $latest, $oldest ) = ( sort { $a->{ mtime } <=> $b->{ mtime } } map { { mtime => -M $_, file => $_ } } <$dir/*> )[ 0, -1 ]; my ( $newM, $oldM ) = map +( stat )[ 9 ], $latest->{ file }, $oldest-> +{ file }; # Looks in $dirL and sets $latestL->{file} with newest file and sets $ +oldestL->{file} with oldest file. my ( $latestL, $oldestL ) = ( sort { $a->{ mtime } <=> $b->{ mtime } } map { { mtime => -M $_, file => $_ } } <$dirL/*> )[ 0, -1 ]; my ( $newML, $oldML ) = map +( stat )[ 9 ], $latestL->{ file }, $oldes +tL->{ file }; # Opens $dir and counts number of files my @files = <$dir/*>; my $fCount = @files; # Opens $dirL and counts, large files are processed seperately thus $d +irL my @filesL = <$dirL/*>; my $fCountL = @filesL; # Changes Newest/Oldest file display in email based on if any files ar +e found. if (defined($latest->{file})){ $latestF = 'Newest File: '. $latest->{file}. ' with a timestamp of + '. scalar localtime $newM; $oldestF = 'Oldest File: '. $oldest->{file}. ' with a timestamp of + '. scalar localtime $oldM; } # Changes Newest/Oldest file display in email based on if any large fi +les are found. if (defined($latestL->{file})){ $latestFL = 'Newest File: '. $latestL->{file}. ' with a timestamp +of '. scalar localtime $newML; $oldestFL = 'Oldest File: '. $oldestL->{file}. ' with a timestamp +of '. scalar localtime $oldML; } # Looking for the lock files generated when pLock(job1) job starts to +see when import started. my $pLock ='\\\\server\\E$\\directory1\\job1.lock'; my $pLockStat='No lock file for Job1 was found, this import is not cur +rently running.'; if ( -f $pLock ) { my @time = timeconv( -M $pLock ); my $pLockWhen=$time[1].' hours, '.$time[2].' minutes, and '.$ +time[3].' seconds ago'; $pLockStat='Lock file found for Job2, this import was started + '. $pLockWhen; } # Looking for the lock files generated when eLock(job2) job starts to +see when import started. my $eLock ='\\\\server\\E$\\directory1\\job1.lock'; my $eLockStat='No lock file for Job2 was found, this import is not cur +rently running.'; if ( -f $eLock ) { my @time2 = timeconv( -M $eLock ); my $eLockWhen=$time2[1].' hours, '.$time2[2].' minutes, and ' +.$time2[3].' seconds ago'; $eLockStat='Lock file found for Job2, this import was started + '. $eLockWhen; }