On Mon, Dec 19, 2011 at 8:09 PM, Shlomi Fish <[email protected]> wrote:
> Hi Jonathan, > > some comments on your code - both positive and negative. > > On Mon, 19 Dec 2011 19:32:10 +0000 > Jonathan Harris <[email protected]> wrote: > > > Hi Perl Pros > > > > This is my first call for help > > > > I am a totally new, self teaching, Perl hopeful > > > > If my approach to this script is simply wrong, please let me know as it > > will help my learning! > > > > The script aims to: > > > > 1) Read in a directory either from the command line, or from a default > path > > 2) Produce a hash for future checksum > > 3) Write this (hex digest) to a separate file, in a sub directory of the > > parent, which has the same name and a .md5 extension > > 4) Check the original file for its size > > 5) Add this data to the newly created file on a new line (in bytes) > > > > I have a script that will do most of this, except for analysing the file > > size - I think that the file size being analysed may be the md5 object > > result as the same value is printed to each file > > > > I am running out of ideas and would appreciate any help you could give! > > I have tried using File::stat::OO and File::stat - but to no avail - I > > could be using them incorrectly! > > > > Many thanks in advance... > > > > Jon. > > > > Here are some details: > > > > System: > > Mac OSX 10.7.2 > > Perl version 5.12 > > > > Script: > > > > #!/usr/bin/perl > > # md5-test-3.plx > > use warnings; > > use strict; > > strict and warnings are good. > > > use Digest::MD5; > > So is using a module. > > > > > my $filesize = 0; > > You shouldn't predeclare your variables. > > > my $dir = shift || '/Users/jonharris/Documents/begperl'; > > opendir (DH, $dir) or die "Couldn't open directory: $!"; > > Don't use bareword dir handles - use lexical ones. It's good that you're > using > the "or die" thing, though. > > > > > my $md5 = Digest::MD5->new; > > Seems like you're using the same $md5 object times and again which will > calculate cumulative MD5 sums instead of per-file ones. > > > while ($_ = readdir(DH)) { > > 1. You're not skipping "." and "..". > > 2. You're not skipping other directories. > > 3. The $_ variable can be easily devastated. You should use a lexical one. > > > $md5->add($_); > > According to http://metacpan.org/module/Digest::MD5 the "add()" methods > adds > data, and here it will only add the filename. You need to use addfile() > with an > opened file handle instead. > > > $filesize = (stat(DH))[7]; > > You shouldn't stat the directory handle. Instead stat "$dir/$filename" > (you can > also use the core File::Spec module if you want to make it extra portable). > > > #### Is it necessary to put the following into a new loop? > > > > foreach ($_) { > > Why the foreach ($_) here? It does nothing. You're already iterating on the > files. > > > open FH, ">> /Users/jonharris/Documents/begperl/md5/$_.md5" or die $!; > > binmode(FH); > > print FH $md5->hexdigest, "\n", $filesize; > > } > > close FH; > > > Use lexical file handles here, use three-args open - «open my $fh, '>>', > '/Users....'» and don't open it time and again. Keep it outside the loop. > > For more information see: > > http://perl-begin.org/tutorials/bad-elements/ > > Regards, > > Shlomi Fish > > > } > > close DH; > > print "\n$dir\n\n"; > > > > ####### > > > > -- > ----------------------------------------------------------------- > Shlomi Fish http://www.shlomifish.org/ > Chuck Norris/etc. Facts - http://www.shlomifish.org/humour/bits/facts/ > > We don’t know his cellphone number, and even if we did, we would tell you > that > we didn’t know it. > > Please reply to list if it's a mailing list post - http://shlom.in/reply . > Hi Shlomi Thanks for your response To answer your questions: > my $filesize = 0; >You shouldn't predeclare your variables Noted, thanks > my $dir = shift || '/Users/jonharris/Documents/begperl'; > opendir (DH, $dir) or die "Couldn't open directory: $!"; > Don't use bareword dir handles - use lexical ones. It's good that you're using > the "or die" thing, though. Of course - will have to re-read the section on barewords... > my $md5 = Digest::MD5->new; > Seems like you're using the same $md5 object times and again which will > calculate cumulative MD5 sums instead of per-file ones. In which case, I think that it will have to go in the loop so that a new instance is produced each time? > while ($_ = readdir(DH)) { > 1. You're not skipping "." and "..". > 2. You're not skipping other directories. I'm sure I read somewhere, that the parent was automatically skipped Must be getting confused However, adding this will be ok: next if $_ eq "." or $_ eq ".."; > 3. The $_ variable can be easily devastated. You should use a lexical one. I'll certainly use lexical in the future > $md5->add($_); > According to http://metacpan.org/module/Digest::MD5 the "add()" methods adds > data, and here it will only add the filename. You need to use addfile() with an > opened file handle instead. That makes sense > $filesize = (stat(DH))[7]; > You shouldn't stat the directory handle. Instead stat "$dir/$filename" (you can > also use the core File::Spec module if you want to make it extra portable). This is where I have been tying my head in knots I'll give that a try Although, I do remember trying and getting an error: use of uninitialised value at line x Probably because the code prior to it was incorrect > foreach ($_) { > Why the foreach ($_) here? It does nothing. You're already iterating on the > files. Thought as much - I tried it both ways and it didn't seem necessary - as you said, I'm already iterating over the files > Use lexical file handles here, use three-args open - «open my $fh, '>>', > '/Users....'» and don't open it time and again. Keep it outside the loop. > For more information see: http://perl-begin.org/tutorials/bad-elements/ Thanks for all your help I'll have a read-up a rethink and a re-write! All the yes Jonathan
