I am writing an article for a software magazine, and the article will be accompanied by some simple Perl scripts. I asked in SoPW for code reviews.
The original node is here.
A number of monks gave me excellent advice, some of which I didn't follow, but all of which I really appreciate!.
(And of course, feel free to keep reviewing, if you see something that offends your sensibilities...)
My revised code follows:
#!/usr/bin/perl #InventoryReport.pl use warnings; use strict; use File::Find; open (OUT, ">InventoryFile.txt") or die "Couldn't open output file\n"; my $directory = '//InstallBox/C'; find (\&wanted, $directory); sub wanted { print OUT "$File::Find::name"; } # # Lines 4 and 5 are good Perl practice. "strict" and "warnings" are p +ragmas # that help prevent Perl programmers from doing silly things in their +programs. # # Line 6 is Perl's way of invoking a "module". Modules are sophisticat +ed chunks # of code with simple interfaces for use by Perl programmers. I don't +have to # write my own recursive filesystem prober, I can just use File::Find +and let # it do the work for me. # # Line 8 "open" statement with the ">" purges any contents if the file + exists. # If there's a problem, the program will "die" with an error msg. # # Edit line 11 so that "my $directory" points to your own top-level di +rectory. # I've used Windows syntax here, but the script works just fine on oth +er OSs. # # Lines 13-16 are the basic implementation of File::Find. # Do "perldoc File::Find" for details. # ######################################################
#!/usr/bin/perl #CompareReport.pl use warnings; use strict; use List::Compare; open (OUT, ">CDiffs.txt") or die "Couldn't open output diffs file\n"; open (FILE1, "OldInventory.txt") or die "Couldn't open old inventory file\n"; open (FILE2, "NewInventory.txt") or die "Couldn't open new inventory file\n"; my @file1 = <FILE1>; $_ = lc for @file1; my @file2 = <FILE2>; $_ = lc for @file2; my $lc = List::Compare->new(\@file1, \@file2); my @file1only = $lc->get_Lonly; my @file2only = $lc->get_Ronly; print OUT "For unique files in OldInventory.txt:\n"; print OUT "IGNORING FILES WITH Tmp OR Temp IN PATHNAME.\n"; print OUT "IGNORING FILES IN THE RECYCLE BIN.\n"; print OUT "IGNORING ALL HISTORY FILES.\n"; print OUT "LOWERCASING ALL FILE AND PATHNAMES BEFORE COMPARING.\n"; print OUT "\n"; foreach my $file1(@file1only) { unless (($file1 =~ "tmp") or ($file1=~"temp") or ($file1 =~ "recycler") or ($file1 =~ "history")) { print OUT $file1; } } print OUT "\n\n"; print OUT "For unique files in NewInventory.txt\n"; print OUT "IGNORING FILES WITH Tmp OR Temp IN PATHNAME.\n"; print OUT "IGNORING FILES IN THE RECYCLE BIN.\n"; print OUT "IGNORING ALL HISTORY FILES.\n"; print OUT "LOWERCASING ALL FILE AND PATHNAMES BEFORE COMPARING.\n"; print OUT "\n"; foreach my $file2(@file2only) { unless (($file2 =~ "tmp") or ($file2=~"temp") or ($file2 =~ "recycler") or ($file2 =~ "history")) { print OUT $file2; } } # # Lines 4 and 5 are good Perl practice. "strict" and "warnings" are p +ragmas # that help prevent Perl programmers from doing silly things in their +programs. # # Line 6 is Perl's way of invoking a "module". Modules are sophisticat +ed chunks # of code with simple interfaces for use by Perl programmers. # # Line 8 "open" statement with the ">" purges any contents if the file + exists. # If there's a problem, the program will "die" with an error msg. The +other # "open" statements are for input, and are not destructive. # Note that this script assumes that the input files are in the direct +ory from # which the script is run. # # Line 15 creates an array "@file1" where each element of the array is + a line of # the file pointed to by "FILE1". # Line 17 does the same thing for FILE2 into the array @file2. # # Lines 16 and 18 are somewhat idiomatic Perl. "lc" is the lowercase +function. # These statementslowercase each element of each array by iterating ov +er each # array using a "for" loop. # # Line 20 cranks up List::Compare. See the documentation on CPAN for +full # details. # # Lines 22 and 23 implement the List::Compare functions that we care a +bout. # "get_Lonly" shows each item unique to the left array, @file1. Since + @file1 # is our old inventory, the array @file1only will contain all of the f +ilenames # left behind from the old inventory. @file2only will contain all of t +he files # added to the new inventory. # # Lines 25-30 are printed to the output file so the user knows what's +going on. # # Lines 32-39 check each element of the @file1only array. "unless" is + a nifty # feature of Perl. "unless" translates as "if not". And the "=~" is t +he Perl # pattern match operator. So these lines could be translated as "Check + each item # in the array. If the item does *not* contain any of these text stri +ngs, print # the item to the output file". # Also note a potential bug here: "temp" will match "Temporary" and "T +emp", but # will also match "ExemptEmployees" and "New Attempt", etc. thus preve +nting them # from being reported. Perl gives you lots of ways to fix the problem +, I'll # leave it to you scripters to decide what needs to be done here! # # Line 39 prints a couple of newlines to separate the sections of the +report. # # Lines 41-54 do the same work for the @file2only array, reporting on +elements # unique to the new inventory. This repeated work should really be ref +actored # as a subroutine. It's not hard, but I didn't want to have to introd +uce that # syntax as well. So I encourage you scripters to make my code better + by using # a subroutine for this repeated work! #
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re: Reviewed code reviewed again
by particle (Vicar) on Apr 15, 2004 at 21:37 UTC | |
by jeffa (Bishop) on Apr 16, 2004 at 04:55 UTC | |
by McMahon (Chaplain) on Apr 16, 2004 at 22:15 UTC | |
by Anonymous Monk on Apr 16, 2004 at 04:52 UTC | |
|
Re: Determine files added/removed (code for magazine article)
by tilly (Archbishop) on Apr 16, 2004 at 05:24 UTC | |
by McMahon (Chaplain) on Apr 16, 2004 at 22:11 UTC | |
|
Re: Determine files added/removed (code for magazine article)
by fluxion (Monk) on Apr 23, 2004 at 22:16 UTC |