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? } }

In reply to Re: code critique by grantm
in thread code critique by puntme

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":



  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.