views:

583

answers:

3

Hello,

I'm learning Perl and at the same time I'm creating a program to my family events, but when I'm trying to use a array with a randomization process I'm getting some errors, as you could see:

[ubuntu@eeepc:~/Desktop/mail] ./get.pl -h pop.vix.terra.com.br -u nathanpc -p  (:D)
Global symbol "$random_name" requires explicit package name at ./get.pl line 17.
Execution of ./get.pl aborted due to compilation errors.
[ubuntu@eeepc:~/Desktop/mail]

And my code is like this:

#!/usr/bin/perl

# import packages
use Net::POP3;
use Getopt::Long;
use Mail::Message;
use List::Util qw(shuffle);
use strict;
use warnings;

# Variable declaration
my $host;
my $user;
my $pass;
my $email_file;
my $msg;
my @array = shuffle(<$random_name>);

# read command line options
# display usage message in case of error
GetOptions ('h|host=s' => \$host,
            'u|user=s' => \$user,
            'p|pass=s' => \$pass) or die("Input error. Try calling me with: -h <host> -u <username> -p <password>");

# file operations
open($email_file, ">>", "Mail.txt");
open my $random_name, "<", "out.txt";

# initiate connection
# default timeout = 120 sec
my $conn = Net::POP3->new($host) or die("ERROR: Unable to connect.\n");

# login
my $numMsg = $conn->login($user, $pass) or die("ERROR: Unable to login.\n");

# get message numbers
# iterate over list and print first 20 lines of each
if ($numMsg > 0) {
    my $msgList = $conn->list();
    foreach $msg (keys(%$msgList)) {
        my $rawdata = $conn->get($msg);
        my $msg_obj = Mail::Message->read($rawdata);
        my $body = $msg_obj->body;
        print $email_file $body;
        print $email_file "\n====================================================\n";
        print shift @array;
    }
} else {
    print "Mailbox is empty.\n";   
}

# close connection
$conn->quit();
close($email_file);
close($random_name);
+3  A: 

On line 17, $random_name is not initialised yet. You will want to put this statement after the $random_name file is opened (line 27).

Greg Hewgill
+5  A: 

This is the line that is causing the problem.

my @array = shuffle(<$random_name>);

You need to define $random_name before using it. Try

open my $random_name, "<", "out.txt";
my @array = shuffle(<$random_name>);
unutbu
I think you have a typo there, should be 'open my $random_name' without the 2
mikegrb
@mikegrb: Ah yes, thanks!
unutbu
+5  A: 

The answers from Greg Hewgill and ~unutbu are correct. I just wanted to add that it's best not to pre-declare variables, that may have helped a tad in understanding what was wrong.

Here is your identical code with some slight changes:

#!/usr/bin/perl

# import packages
use Net::POP3;
use Getopt::Long;
use Mail::Message;
use List::Util qw(shuffle);
use strict;
use warnings;

# read command line options
# display usage message in case of error
my ($host, $user, $pass);
GetOptions ('h|host=s' => \$host,
            'u|user=s' => \$user,
            'p|pass=s' => \$pass) or die("Input error. Try calling me with: -h <host> -u <username> -p <password>");

# file operations
open (my $email_file, ">>", "Mail.txt") or die ("Error opening Mail.txt for write: $!");
open (my $random_name, "<", "out.txt") or die ("Error opening out.txt for read: $!");
my @array = shuffle(<$random_name>);
close($random_name);

# initiate connection
# default timeout = 120 sec
my $conn = Net::POP3->new($host) or die("ERROR: Unable to connect.\n");

# login
my $numMsg = $conn->login($user, $pass) or die("ERROR: Unable to login.\n");

# get message numbers
# iterate over list and print first 20 lines of each
if ($numMsg > 0) {
    my $msgList = $conn->list();
    foreach my $msg (keys(%$msgList)) {
        my $rawdata = $conn->get($msg);
        my $msg_obj = Mail::Message->read($rawdata);
        my $body = $msg_obj->body;
        print $email_file $body;
        print $email_file "\n====================================================\n";
        print shift @array;
    }
} else {
    print "Mailbox is empty.\n";
}

# close connection
$conn->quit();
close($email_file) or die "Error closing Mail.txt from write: $!";
  • I removed the predeclaration of variables.
  • I changed the two opens to both use parenthesis and both check for errors.
  • I moved declaring and setting @array to just after out.txt is opened.
  • Since $random_file is not needed after @array is set, I close it on the next line.
  • Lastly, I check for errors when closing Mail.txt which was opened for writing. It is very important to check the return value of close on a file you opened for writing as certain errors, like running out of disk space while writing to the file, will not be seen in the initial open but will be visible by checking that close($fh) returned true.

There is still room for improvement but those were the biggies. I must say though, your code was a pretty good start for someone new to Perl. Using use strict and warnings, the foreach loop to iterate over the keys of the hash as well as Getopt::Long vs trying to parse command line arguments yourself are good to see.

mikegrb
I would just use the autodie pragma and have all the error checking done automatically; it also improves readability a lot since there is not so much error-handling code, which can be distracting.
brunov
+1 I agree and use autodie quite a bit myself but I think there is value showing the do it yourself method to someone new... If nothing else so that the properly appreciate autodie later ;)
mikegrb
Thanks very much, you have explained very nice! ;)
Nathan Campos

related questions