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;'
}

[download]

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

In reply to Re: Pass the arguments to the procedure in an easier way by davido
in thread Pass the arguments to the procedure in an easier way by Scottie

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.