Beefy Boxes and Bandwidth Generously Provided by pair Networks
P is for Practical
 
PerlMonks  

Is this the most elegant way to code directory lookup?

by texasperl (Sexton)
on Sep 29, 2006 at 14:03 UTC ( #575529=perlquestion: print w/replies, xml ) Need Help??

texasperl has asked for the wisdom of the Perl Monks concerning the following question:

Hi. I'm more or less new to perl, and while this code is functional and it does what I want it to do...my question is, is this the "best" way? Are there some additional caveats and/or shortcuts I could be using here?
my $srcdir = '/var/local/some_dir/'; my $mtime = '1'; my $destdir = '/backup/some_dir/'; unless (-d $srcdir && -d $destdir ) {die "Error: $!";} print "Beginning backup...\n"; chdir $srcdir; opendir(INDIR,$srcdir) || die "Can't open directory: $!"; my @files = readdir(INDIR); closedir (INDIR); foreach (@files) { unless (-d || $_ eq "." || $_ eq "..") { if (-M > $mtime) { print "$_ : Older than one day.\n"; } else { `tar -cz $_ -f $_.tar.gz`; `mv *.gz $destdir`; print "$_ archived in $destdir\n"; } } }
Many thanks in advance.

2006-09-30 Retitled by planetscape, as per Monastery guidelines: one-word (or module-only) titles hinder site navigation

( keep:1 edit:22 reap:0 )

Original title: 'Elegance?'

Replies are listed 'Best First'.
Re: Is this the most elegant way to code directory lookup?
by davorg (Chancellor) on Sep 29, 2006 at 14:15 UTC

    Three things that I would change:

    I wouldn't read the filenames into an array, I'd read them one at a time.

    while ($_ = readdir(INDIR)) { ... }

    I'd use next to skip the loop for '.' and '..'.

    while ($_ = readdir(INDIR)) { next if -d || $_ eq '.' || $_ eq '..'; ... }

    Finally (and most importantly) using backticks for commands where you're not capturing the output is wasteful as Perl collects up all the output and returns it to you - only for you to throw it away. Better to use system in that case. Or to use Perl version of the command where they are available - like using move (from File::Copy instead of mv).

    Update: One more thing I've just noticed.

    unless (-d $srcdir && -d $destdir ) {die "Error: $!";}

    Not sure why you're including the value of $! in that error message. It won't contain a value at that point.

    --
    <http://dave.org.uk>

    "The first rule of Perl club is you do not talk about Perl club."
    -- Chip Salzenberg

      Not sure why you're including the value of $! in that error message. It won't contain a value at that point.
      I thought that, too. I tested it, however, and

      $ perl -e '-e q{non-existent} or die $!;' No such file or directory at -e line 1. $

      Cheers,

      JohnGG

      "Finally (and most importantly) using backticks for commands where you're not capturing the output is wasteful as Perl collects up all the output and returns it to you - only for you to throw it away. Better to use system in that case. Or to use Perl version of the command where they are available - like using move (from File::Copy instead of mv)."

      Be thankful that Perl throws it away. If you do the same thing in (e.g.) ksh then the command for the shell to execute is evaluated as the return from the back-ticked command. i.e. it doesn't throw the output away it executes it!

      Now I can see how this might be useful in some obscure circumstances.....however I have seen back-ticks used without the realization of the implications :-(

      And yes, it did take me a while to work out where the obscure error messages were coming from!

      Cheers

      Dave.R

      Nothing succeeds like a budgie with no teeth.

      2006-10-03 Retitled by planetscape, as per Monastery guidelines: one-word (or module-only) titles hinder site navigation


      Original title: 'Re^2: Backticks'

Re: Is this the most elegant way to code directory lookup?
by Hue-Bond (Priest) on Sep 29, 2006 at 14:12 UTC

    Some comments:

    unless (-d $srcdir && -d $destdir )

    It's better not to use unless, unless for simple conditionals ;^).


    opendir(INDIR,$srcdir) || die

    Better use lexical filehandles. Also, I don't like to use parentheses, it makes the code more perlish:

    opendir my $indir, $srcdir or die "blah: $!";

    unless (-d || $_ eq "." || $_ eq "..") {

    Same as above. Quick, what does this do?

    unless ($bar =~ /bar/ or $baz !~ /qux/ and defined $undef)

    `tar -cz $_ -f $_.tar.gz`;

    Use backticks when you are going to use the output of the command. This is not the case, so you better use system.

    Update: Oh, there's a recent thread about unless, be sure to check it out.

    --
    David Serrano

      Quick, what does this do?
      unless ($bar =~ /bar/ or $baz !~ /qux/ and defined $undef)

      Exactly the same as

      if( not ($bar =~ /bar/ or $baz !~ /qux/ and defined $undef) )

      There. So much clearer now...not.


      Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
      Lingua non convalesco, consenesco et abolesco. -- Rule 1 has a caveat! -- Who broke the cabal?
      "Science is about questioning the status quo. Questioning authority".
      In the absence of evidence, opinion is indistinguishable from prejudice.
        BrowserUk++

        A not, a regex, an or and an and. Not only not clear but almost impossible for me to get my head round.

        I resort to extreme lengths to try and get it straight.

        if( not ( ( $bar =~ /bar/ or ( $baz !~ /qux/ and defined $undef ) ) ) )
        But I find that almost always these constructions are the result of a poor algorithm. As the old saying goes "get your algorithm right and the code writes itself". While we're here, that is closely followed by "don't try to code yourself out of a problem, start again".

        It works for me! :-)

        There. So much clearer now...not.

        Your ability to make something less clear doesn't negate everyone else's ability to make something more clear.

        if( ! -d && $_ ne "." && $_ ne ".." ) {

        Yes, that prevents a noticeable mental pause that the "unless" version induced. [ Even better, it avoids the mental tension between how "unless" is usually used in English vs. how it is used in Perl and the fact that the unexplicit "not" implied by "unless" is too easy to mentally drop. ]

        - tye        

        You made Hue-Bond's point. The statement was hard to understand without some major transformations. For example, the following is clearer:
        if ( $bar !~ /bar/ and ( $baz =~ /qux/ or not defined $undef ) )

        But let's not stop there. Reduce the number of things the reader has to keep in mind by breaking the statement into two.

        if( $bar !~ /bar/ ) if ( $baz =~ /qux/ or not defined $undef )
Re: Is this the most elegant way to code directory lookup?
by texasperl (Sexton) on Sep 29, 2006 at 15:12 UTC
    With your helpful suggestions I have modified my code as follows. Better? I hope so. =]
    #!/usr/bin/perl use warnings; use strict; use File::Copy; # change source backup dir, modification time, and destination backup +dir here my $srcdir = '/var/local/somedir'; my $mtime = '1'; my $destdir = '/backup/somedir'; die "Error:$!" unless (-d $srcdir && -d $destdir ); print "Beginning backup...\n"; chdir $srcdir; opendir(INDIR,$srcdir) || die "Can't open directory: $!"; while ($_ = readdir(INDIR)) { next if (-d || $_ eq "." || $_ eq ".."); if (-M > $mtime) { print "$_ : Older than one day.\n"; } else { system("tar -cz $_ -f $_.tar.gz"); move(<*.gz>, $destdir) && print "$_ archived in $destd +ir\n"; } } closedir(INDIR); print "Backup complete.\n";
      die "Error:$!" unless (-d $srcdir && -d $destdir );

      As I mentioned before, $! won't have anything useful in it at this point, so there's no reason to include it in the error message.

      Why not make the error messages a little clearer.

      die "$srcdir is not a valid directory\n" unless -d $srcdir; die "$destdir is not a valid directory\n" unless -d $destdir;

      Update: Looks like I'm wrong.

      --
      <http://dave.org.uk>

      "The first rule of Perl club is you do not talk about Perl club."
      -- Chip Salzenberg

        Update: Looks like I'm wrong.

        I disagree: just because someone tried it once and got a useful error message doesn't change the fact that $! is not documented to be defined at this point.

        $! may well get set differently (or not at all) in different circumstances, when testing different files, or on a different O/S, or depending on phase of the moon for all we know.

        Your suggested clearer error messages are definitively more correct; the only change I might make (at the risk of getting too clever) is to avoid the duplication with something like:

        die "$_ is not a valid directory\n" for grep !-d, $srcdir, $destdir;

        Hugo

Re: Is this the most elegant way to code directory lookup?
by jdporter (Chancellor) on Sep 29, 2006 at 16:03 UTC

    I would eliminate the use of chdir. Yes, the program as given works, but only because both of the directories are specified as absolute. I think you'd gain flexibility by allowing relative paths (for example, "." as the srcdir).

    Here's how I might rewrite it:

    #!/usr/bin/perl use Getopt::Long; use strict; use warnings; $|=1; my $srcdir; my $destdir; my $mtime = 1; GetOptions( 'srcdir=s' => \$srcdir, 'destdir=s' => \$destdir, 'mtime|days=i' => \$mtime, ); defined $srcdir && defined $destdir or die <<EOF; Usage: $0 --srcdir SRCDIR --destdir DESTDIR [-days N] EOF opendir SRCDIR, $srcdir or die "opendir $srcdir - $!"; my @too_old; my @files; # only select non-hidden regular files: for ( grep { -f "$srcdir/$_" } readdir SRCDIR ) { if ( -M "$srcdir/$_" > $mtime ) { push @too_old, $_; } else { push @files, $_; } } closedir SRCDIR; @too_old and print "(Ignoring ".scalar(@too_old)." too-old files.)\n"; @files or die "No backup candidates in $srcdir."; unless ( -d $destdir ) { warn "Destination directory $destdir does not exist; attemptin +g to create.\n"; mkdir $destdir or die "mkdir $destdir - $!"; } print "Backing up ".scalar(@files)." files from $srcdir.\n"; for my $f ( @files ) { print "Archiving $f \n"; system qq( cd "$srcdir" ; tar -cz "$f" -f "$f.tar.gz" ); system qq( mv "$srcdir/$f.tar.gz" "$destdir" ); }

    This doesn't entirely avoid chdir, but it puts it at the lowest, most constrained context possible. In fact, if you have gnu tar (which it appears you do), you can exploit the -C option to make tar itself do the directory change within itself.

    We're building the house of the future together.
Re: Is this the most elegant way to code directory lookup?
by texasperl (Sexton) on Sep 29, 2006 at 17:13 UTC
    Wow. I didn't think I'd get so many replies. I'll study your code (and of course, some documentation with it!) and put this stuff to use...thanks monks!
Re: Is this the most elegant way to code directory lookup?
by shmem (Chancellor) on Sep 29, 2006 at 17:15 UTC
    - unless (-d $srcdir && -d $destdir ) {die "Error: $!";} + for($srcdir,$destdir) { ! -d $_ and die "Directory '$_' - $!\n" }

    You wouldn't know which directory is missing.

    - chdir $srcdir; - opendir(INDIR,$srcdir) || die "Can't open directory: $!"; + chdir $srcdir or die "Can't chdir to '$srcdir': $!\n"; + open my $indir, '.' or die "Can't read '.' in $srcdir: $!\n";

    Point is, you are already in $srcdir, so you are here->.

    Instead of using the ugly and hard to grok

    unless (-d || $_ eq "." || $_ eq "..") { # . and .. are always -d

    in the loop, after having lumped all directory entries (files, directories, symlinks, sockets, device files) into @files (are you really looking for files only? and making tar.gz's containing a single file each?) it would be more elegant

    my @files = grep { -f } readdir $indir;

    to stuff only the interesting files into @files.

    `tar -cz $_ -f $_.tar.gz`; `mv *.gz $destdir`;

    Really? what if your directory contains *.gz files? Your asking tar to complain. And use system, not backticks.

    I would pack all files into one tar file:

    chdir $srcdir or die "Can't chdir to '$srcdir': $!\n"; open my $indir, '.' or die "Can't read '.' in $srcdir: $!\n"; (my $tarfile = $srcdir) =~ s|.*/||; $tarfile = "$destdir/$tarfile-" . time . '.tar'; my @files = grep { -f and -M <= 1 } readdir $indir; # packing all files at once doesn't work with large directories # (limit of command line length) while(my $file = shift @files) { system ('tar', 'uf', $tarfile, $file) and die "Can't pack '$file' into '$tarfile' (exitcode $?)\n"; } system ('gzip', $tarfile) and die "Couldn't gzip $tarfile (exitcode $?)\n";

    --shmem

    _($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                                  /\_¯/(q    /
    ----------------------------  \__(m.====·.(_("always off the crowd"))."·
    ");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}
Re: Is this the most elegant way to code directory lookup?
by ChemBoy (Priest) on Sep 29, 2006 at 18:23 UTC

    I hate to add anything that might be construed as fuel to the unless discussion, but aren't "." and ".." both directories anyway? (And, for that matter, if they weren't, would you want to skip them?)



    If God had meant us to fly, he would *never* have given us the railroads.
        --Michael Flanders

Re: Is this the most elegant way to code directory lookup?
by jasonk (Parson) on Sep 29, 2006 at 16:29 UTC

    Try this on for size... Probably still not the way I would approach the problem, but at least it's a bit more elegant...

    my $srcdir ='/var/local/some_dir'; my $mtime = 1; my $destdir = '/backup/some_dir'; # Yours doesn't tell you which directory didn't exist... die "$srcdir does not exist" unless -d $srcdir; die "$destdir does not exist" unless -d $destdir; print "Beginning backup...\n"; # Error checking on chdir is important too, the directory # might exist, but not be readable by you chdir $srcdir or die "Can't change to $srcdir: $!"; # Once your program gets bigger than a couple of lines, # you really want to know which directories or files # couldn't be opened opendir ( INDIR, $srcdir ) or die "Can't open $srcdir: $!"; # if this is a big directory, slurping all the files into # memory can be quite a waste of memory... while ( my $file = readdir( INDIR ) ) { next if -d $file; # -d will be true for . and .. too if ( -M $file > $mtime ) { print "$file : Older than one day.\n"; next; } # tar is for combining multiple things into one, # gzip is for compressing, since you are only doing # one file at a time, the tar is just wasted effort system( "gzip -c $file > $destdir/$file.gz" ); if ( $? != 0 ) { die "Gzip failed" } print "$file archived in $destdir\n"; }

    This is probably how I would actually approach the problem... Although this approach may not meet some requirements that couldn't really be inferred from your code...

    use Path::Class qw( dir ); my $srcdir = dir( '/var/local/some_dir' ); my $destdir = dir( '/backup/some_dir' ); $destdir->mkpath unless -d $destdir; die "$srcdir does not exist" unless -d $srcdir; die "$destdir does not exist" unless -d $destdir; print "Beginning backup...\n"; while ( my $file = $srcdir->next ) { next if $file->is_dir; my $target = $destdir->file( $file->basename.".gz" ); if ( $file->stat->mtime <= $target->stat->mtime ) { print "backup is more recent than original\n"; next; } system( "gzip -c $file > $target" ); if ( $? != 0 ) { die "Gzip failed" } print "$file archived in $destdir\n"; }

    We're not surrounded, we're in a target-rich environment!
Re: Is this the most elegant way to code directory lookup?
by Anonymous Monk on Sep 29, 2006 at 15:31 UTC
    If I understand this correctly, the program takes every file in /var/local/some_dir/ that isn't older than a day, and tars it up in /backup/some_dir/, right? I'm not sure I understand the reason to tar up each file into a different archive, but I wouldn't do it in Perl, but I'd use the shell instead. Perhaps (untested):
    cd /var/local/some_dir find . -type -f -mtime 1 -maxdepth 1 \ -exec tar cfz /backup/some_dir/{}.tar.gz {} \;
    But I guess I'd backup all the files in a single archive; then I'd use find (as above) to list the files, and pipe that into cpio to create the archive.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://575529]
Approved by Old_Gray_Bear
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others taking refuge in the Monastery: (6)
As of 2022-09-30 15:29 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    I prefer my indexes to start at:




    Results (126 votes). Check out past polls.

    Notices?