current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

This is my first Perl script, so far it works fine, except after running for a long time, it begins to throw an odd error: "Use of uninitialzed value at string ne", and it refers to a line in the isdir function in Net::FTP::File.

sub Net::FTP::isdir {
    my $ftp = shift;
    local $_fatal;
    my $c = $ftp->pwd();
    my $r = $ftp->cwd(@_);
    my $d = $ftp->cwd($c);
    my $e = $ftp->pwd();
    $setmsg->( $ftp, "Could not CWD into original directory $c" ) if $c ne $e || !$d;
    return undef if $_fatal;
    return $r ? 1 : 0;
}

And here is my code: 

#!/usr/local/bin/perl

use Net::FTP;
use Net::FTP::File;

$hostname = 'ftp.census.gov';
$username = 'anonymous';
$password = '-anonymous@';

$base_dir = '/geo/tiger';
$root_dir = 'TIGER2010';

$ftp = Net::FTP->new($hostname)
 or die "Failed to connect to ", $hostname;

$ftp->login($username, $password)
 or die "Failed to login ", $ftp->message;

$ftp->cwd($base_dir);

$ftp->binary;

recursive_get($root_dir);

$ftp->quit;

sub recursive_get {

    $ftp->cwd($_[0]);

    print "Adding directory ".$_[0].".\n";  
    mkdir($_[0], 0777) unless -d $_[0];
    chdir($_[0]);

    my @directory_listing = $ftp->ls;

    foreach(@directory_listing) {
        next if ($_ =~ /^\./);

        print "Processing ".$_.".\n";

        recursive_get($_) if $ftp->isdir($_);

        if($ftp->isfile($_)) {
            print "Getting $_ \n";
            $ftp->get($_);
            print "Extracting ".$_."\n";
            system("unzip -u $_");
            unlink($_);
        }
    }
    $ftp->cdup; 
    chdir("..");
}

Thanks in advance, and any general comments about the code would be appreciated too.

share|improve this question
add comment

2 Answers

up vote 6 down vote accepted

On your code itself:

#!/usr/local/bin/perl

Should be:

#!/usr/bin/env perl

Every perl script should

use strict;
use warnings;

Turning on these two might even help you diagnose your error.

A side-effect of turning on strict and warnings is that you must declare your variable scope using my

my $hostname = 'ftp.census.gov';
my $username = 'anonymous';
my $password = '-anonymous@';

my $base_dir = '/geo/tiger';
my $root_dir = 'TIGER2010';
my $ftp = ...

You should unpack the variables in your sub, rather than accessing @_ directly.

my $root_dir = shift;
or
my ($root_dir) = @_;

Whichever version you prefer is fine, I like the second as it makes it easier to add more parameters later on if necessary.

You don't need to interpolate like this:

print "Processing ".$_.".\n";

You can just do:

print "Processing $_\n";
share|improve this answer
    
Great, thanks very helpful tips. I'll make the changes and see if my bug becomes clear. –  Tim Feb 5 '11 at 20:09
    
There are reasons for, and against using #!/usr/bin/env perl. So switching to it isn't always as reasonable as you seem to think. For example, if your hosting some code on CPAN you should use #!/usr/local/bin/perl. –  Brad Gilbert Dec 16 '11 at 23:42
add comment
up vote 1 down vote

You got excellent advice from @Glass.

I'd add that I'm not clear why the code uses local $_fatal; instead of my $_fatal;...there must be a reason, but I very seldom use local and when I do, it is to work with one of the Perl special variables (for example, localizing $/ to slurp a file).

The source of $setmsg is also not clear.

Your code could get hopelessly confused if one of the directories it goes to is a symlink, because the cd .. may not get you back to where you started from. Look up the File::Find module. However, it may not be directly applicable because it assumes a local file system, not an FTP-mediated remote file system.

share|improve this answer
    
Thanks @Jonathan, I didn't worry about symlinks since this script was meant for a very specific purpose (pull down TIGER data), and I know there aren't any symlinks in there. I'll check out File::Find though, it may make sense to make this script more rigorous for future use. –  Tim Feb 7 '11 at 16:31
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.