Find the answer to your Linux question:
Results 1 to 3 of 3
I'm new to Perl so I'm guessing that my script is less than perfect. I was hoping to get a critique of it with thoughts on what I could do ...
Enjoy an ad free experience by logging in. Not a member yet? Register.
  1. #1
    Just Joined! phoch00's Avatar
    Join Date
    Sep 2006
    Location
    Dallas, TX
    Posts
    32

    Perl, Files and Directories -Newbie question


    I'm new to Perl so I'm guessing that my script is less than perfect. I was hoping to get a critique of it with thoughts on what I could do differently. It works as is and seems pretty fast.

    Objective: Recurse through a set of directories and list all files in the format:

    dirname:filename:extension:filesize:date

    and write to a file. That file becomes the basis of a load to a MySQL table.

    Script:

    Code:
    #!/usr/bin/perl
    # directory.pl
    
    use strict;
    use File::Find;
    use Time::localtime;
    use File::Basename;
    
    @ARGV = qw(.) unless @ARGV;
    
    open(TEMPFL, '>/home/phoch/music.txt');
    
    find sub {
    	next if $_ eq "." or $_ eq ".." or -d $_;
    	my $dir = $File::Find::dir;
    	my $age = (stat($_))[9];
    	my $tm = localtime($age);
    	my $ext = (fileparse($_, '\..*'))[2];
    	$ext =~ s/^\.//;
    	print TEMPFL "$dir";
    	print TEMPFL qw(:);
    	print TEMPFL $_,;
    	print TEMPFL qw(:);
    	printf TEMPFL $ext;
    	print TEMPFL qw(:);
    	print TEMPFL -s _ if -r _ and -f _;
    	print TEMPFL qw(:);
    	printf TEMPFL ("%04d-%02d-%02d", $tm->year+1900, $tm->mon+1, $tm->mday);
    	print TEMPFL "\n";
    	}, @ARGV;
    
    close(TEMPFL);
    I learned a bunch by writing this script but I'd love to know what/where I screwed up.

  2. #2
    Linux Guru Cabhan's Avatar
    Join Date
    Jan 2005
    Location
    Seattle, WA, USA
    Posts
    3,252
    Alrighty. It looks pretty good, actually. A few suggestions:

    1) You didn't include an 'or die' statement with the open(). If the open fails, your script will crash later on. The line should be more like:
    Code:
    open(TEMPFL, '>/home/phoch/music.txt') or die "$0: Cannot open /home/phoch/music.txt for output!\n";
    This way, we get a very easy-to-understand error message.

    2) Your print statements are, frankly, disgusting. You can combine them into this very easily:
    Code:
    print TEMPFL "$dir:$_:$ext:"
    print TEMPFL -s if -r and -f;
    printf TEMPFL ":%04d-%02d-%02d\n", $tm->year+1900, $tm->mon+1, $tm->mday);
    Note how we combine them all into just 3 statements. Also note the second statement: $_ is usually the default argument, so oftentimes you don't actually need to name it.

    3) This last one is something that isn't necessary, and in fact one that I rarely use myself. But since you only output to TEMPFL, you can actually just make TEMPFL the default stream, and therefore not have to specify it. By placing the line:
    Code:
    select TEMPFL
    after the open() call, you can remove TEMPFL from the print statements. But again, this is just an idea, not a critique so much.


    I hope that helps!

  3. #3
    Just Joined! phoch00's Avatar
    Join Date
    Sep 2006
    Location
    Dallas, TX
    Posts
    32
    Wow! Thanks a lot. I really appreciate the feedback. You were right, my Print statements were gross. It evolved that way I guess because I was going through an iterative process to figure out each piece (i.e. extension, size, etc). so I built the lines one at a time to get the result I wanted. I should have revisited it after to consolidate where possible. Thanks for pointing that out. I'll also give the "select TEMPFL" a try. Sounds like a much cleaner approach.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •