Hi Jonathan,
some comments on your code - both positive and negative.
On Mon, 19 Dec 2011 19:32:10 +0000
Jonathan Harris <jtnhar...@googlemail.com> 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 .
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
