in reply to Pass the arguments to the procedure in an easier way

One issue is that you're returning on a rejected file before finishing the list of files, and doing so without closing the filehandle. Since it's a bareword filehandle, it won't close automatically either. Use lexical filehandles, and don't return (just move onto the next file) in case of a rejected file.

Some of the logic for rejecting files can be moved up out of the loop by grepping to obtain only those files you care about to begin with. It's a big "who cares" with respect to efficiency (introducing the grep loop while moving tests out of the inner loop) because it's doubtful that you're dealing with enough files for the loops to bog down. I just liked the clarity of eliminating rejects from the list being looped over, rather than dealing with them inside the loop. Obviously I did some checking inside the loop, where it would have impeded clarity to move it into the grep.

I switched you over to lexical filehandles which will close themselves if they pass out of scope. But because it's an outfile you're dealing with I still retained a close, so that I could test for failure. You could have used the autodie pragma instead though.

Once I started moving things around it seemed like some of the complexity cleared itself up. I don't think I altered your functionality, though I haven't tested. You would want to give it the solid run-through before depending on it not altering functionality. One thing that did change, is it will not return "" on failure. Instead, the hash will be empty.

Just look over the code. It should make sense. I introduced another checks for you: -f (are we dealing with a file?).

sub md5files { my $dir = shift; # where the files are (must be a # direct path from root /) my $what = shift || '*'; # what files, default = '*' my $md5file = shift || "$dir/checksums.md5"; # default. my %hash_md5; my @t = split(/ /, $what); open (my $md5_outfh, '>', $md5file ) or die "Err: $!"; foreach my $f ( @t ) { foreach my $file2md5 ( grep { -f and $_ ne $md5file } <$dir/$f> ) { next unless my $md5 = md5calc( $file2md5 ); my ( $filename ) = $file2md5 =~ m{/([^/]+)$}; say $md5_outfh "$md5\t$filename"; $hash_md5{$md5} = $filename; } } close $md5_outfh; return %hash_md5 or die $!; # Consider 'use autodie;' }

Oh, also I couldn't see where you were using any functionality from File::Basename, so for the time being I commented it out (and removed it from within the sub).


Dave