On Thu, Dec 29, 2011 at 5:08 PM, Igor Dovgiy <ivd.pri...@gmail.com> wrote:
> Hi Jonathan,
>
> Let's review your script a bit, shall we? )
> It's definitely good for a starter, but still has some rough places.
>
> #!/usr/bin/perl
>> # md5-test.plx
>> use warnings;
>> use strict;
>>
> use File::Find;
>>
> use Digest::MD5;
>> use File::Spec;
>>
> So far, so good. )
>
>
>> my $dir = shift || '/Users/jonharris/Documents/begperl';
>>
> Nice touch, setting up a default param. ) The name of variable might seem
> too generic to some, but then again, it's the only directory we deal with,
> so...
>
>
>> my ($dircontents, $path, @directory, $fh, $wr_fh);
>>
> Incoming!
>
> Well, it's usually better to declare your variables right before you'll
> really need them...
> Your script is short, so you'll hardly have a chance to forget what $fh
> and $wr_fh mean, though. )
>
>
>> @directory = $dir;
>> $path = File::Spec->catfile( @directory, $dircontents );
>>
> Ahem. At least three 'wtf' moments for me. )
> First of all, File::Spec->catfile is really just a glorified join operator
> with some additional operations depending on which system you're using.
> So, second, it makes a little sense to convert $dir into @directory
> (documentation example is just that, an example) and to pass there
> undefined $dircontents as well.
> But the major one is why you'd ever have to pass your $dir through
> File::Spec? It's, you know, user input... )
>
> opendir (my $in, $path) or die "Cannot open $dir: $!\n";
>>
> So you're trying to open $path, but warn about failure to open $dir? )
> But then again, that's a minor quarrel, considering this:
>
>
>> find (\&wanted, $path);
>>
> See, File::Find is convenient method which _emulates_ the whole
> opendir-readdir-closedir pattern for a given directory.
> The 'wanted' subroutine (passed by ref) will be called for each file found
> in $path.
> It's described really well in perldoc (perldoc File::Find).
>
> close $in;
>>
> Opendir, but close - and not closedir? Now I'm confused. )
>
> opendir (my $totalin, $path) or die "Cannot open $dir: $!\n";
>> find (\&cleanup, $path);
>> close $totalin;
>>
> You don't have to use different variable to store temporary file handle
> (which will be closed in three lines) - and that will save you a few
> moments spent working out a new (but rational) name for it. :)
> But then again, you don't need to open the same dir twice: you can call
> cleanup (with the same 'find (\&cleanup)... ' syntax) whenever you want.
> And you don't really need cleanup... whoops, going too far too soon. :)
>
> print "\n\nAll Done!\n\n";
>>
>
>> sub wanted {
>> while ($dircontents = readdir($in)) {
>>
> Did I say that you're using two alternative methods of doing the same
> thing? )
> But there's another big 'no-no' here: you're using external variable
> ($dircontents) when you really have perfectly zero reasons to do so.
> Of course, you don't need to push dirhandle ($in) from outer scape into
> this sub, when using find... ($File::Find::dir will do), but that's
> explainable at least. )
>
> if ($dircontents=~/^\./ || -d $dircontents) {
>> next;
>> }
>>
> So now the script ignores all the files which names begin with '.', and
> you really wanted just to ignore '.' and '..' ... )
>
> my $bytes = -s $dircontents;
>>
> print $dircontents, "\n";
>> print $bytes, " bytes", "\tSo far so good!\n";
>>
> Yes. )
>
>> open $fh, "<", "$dircontents" or die $!;
>> open $wr_fh, "+>", "$path $dircontents.md5" or die $!; ## was unable to
>> concatenate here, hence sub cleanup to remove the ' '
>>
> What was wrong with ...
> open my $wr_fh, '>', File::Spec->catfile($path, $dircontents, '.md5') or
> die $!, $/
> ?
>
>>
>> my $hex = Digest::MD5->new->addfile($fh)->hex digest;
>> print "Hex Digest: ", $hex, "\n\n";
>> print $wr_fh $hex, "\n", $bytes, "\n\n";
>>
> All looks great for now: you're calculating md5 and size, and writing it
> into file with md5 extension...
>
>> return($hex);
>>
> ... but now you're abruptly jumping out of the while block, making the
> whole point of cycle, well, completely pointless. Not great.
>
>> close $wr_fh;
>> close $fh;
>>
> }
>> }
>>
>
>
>>
>> # The following is mostly not original code - thanks to the author!
>>
> sub cleanup {
>> my @filelist = readdir($totalin);
>> foreach my $oldname (@filelist) {
>> next if -d $oldname;
>> my $newname = $oldname;
>> $newname =~ s/\ //;
>>
> So you don't have spaces in your filenames. Great for you. )
>
>> rename $oldname, $newname;
>> }
>> }
>>
>>
> ##### End #####
>>
>>
> And here we finish.
>
> Computers are not smart. They're dumb. But they're fast. And obedient. )
> That's why they're really helpful in letting you do what you're trying to
> do... but only if you KNOW what you're trying to do.
>
> Imagine that you're - and not your computer - will be doing this task.
> Sit in one place - and write down your program as you (and not your
> computer) will be running it. Step by step. Bit by bit.
>
> Then convert your notes into some Perl form - and you'll instantly see the
> difference between now and then. )
>
> -- iD
>
Hi Igor
Many thanks for your response
I have started reviewing the things you said
There are some silly mistakes in there - eg not using closedir
It's a good lesson in script vigilance
I found the part about opening the file handle particularly interesting
I had no idea that
open my $wr_fh, '>', File::Spec->catfile($path, $dircontents, '.md5') or
die $!, $/
was possible
Now it's time to sit down and digest all this......and rewrite the script
to make it better!
Many thanks for your advice and Happy New Year
Jonathan
