Beefy Boxes and Bandwidth Generously Provided by pair Networks
The stupid question is the question not asked
 
PerlMonks  

Re: code critique

by grantm (Parson)
on May 28, 2011 at 01:07 UTC ( [id://907088]=note: print w/replies, xml ) Need Help??


in reply to code critique

As you get more experienced, you will find yourself using fewer global variables and eliminating side effects from functions as much as possible. For example your date() function operates entirely on side effects (taking 'input' from one global variable and writing output into another) rather than taking arguments and returning a result. When someone reads this section at the start of your script ...

if ($date) { date(); }

it's not at all obvious what the date() function will do. It would be clearer written as:

if ($date) { $when = date($date); }

Also 'date' is probably not the most descriptive name you could come up with. Perhaps this might be better:

sub format_date_mdy { my($date) = @_ my $dt = parsedate($date); return strftime("%m%d%y", localtime($dt)); }

Similarly, other functions that needs some data shoul have that data explicitly passed in as named arguments), e.g.:

login($command, $login, $password); sub login { my($command, $login, $password) = @_; ... }

Are you sure you're grabbing the correct variables off the command line? Remember the first element in the @ARGV array is $ARGV[0] not $ARGV[1]. And, as has been pointed out, you really don't want to be accessing @ARGV until after you call getopts() because it will strip out any options. Also, if your script requires each of these variables then it should probably die if they're not provided:

my $job = shift or die "Need a job name";

Another way to handle error messages is using the Pod::Usage module that ships with Perl. This allows you to output a specific error message followed by the usage statement:

my $job = shift or pod2usage( -message => "Need a job name", -verbose => 0, -exitval => 1, );

Your code would be easier to follow if you tidied up the indentation (a decent editor can help you with this). For example, the first option handling block would look better as:

if ($options{t}) { if ($job && $queue && $date && $time) { print "At $time on $when, this will run $job in queue $queue. +\n"; } elsif ($job && $queue && $date) { print "Please enter a time for the job to run. \n"; # Shouldn't we be exiting at this point? } elsif ($job && $queue) { print "This will run $job in queue $queue immediately with the + -n flag \n"; } elsif ($job eq "install") { print "OK\n"; } else { print "Please specify a Job Name and Queue \n"; # Shouldn't we be exiting at this point? } }

Log In?
Username:
Password:

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

How do I use this?Last hourOther CB clients
Other Users?
Others scrutinizing the Monastery: (7)
As of 2024-04-16 08:17 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found