Beefy Boxes and Bandwidth Generously Provided by pair Networks
go ahead... be a heretic
 
PerlMonks  

code critique

by puntme (Initiate)
on May 27, 2011 at 19:48 UTC ( #907061=perlquestion: print w/ replies, xml ) Need Help??
puntme has asked for the wisdom of the Perl Monks concerning the following question:

I was wondering if anyone would be willing to critique my code. I'm learning Perl as my first programming language and I'm sure theres tons of things I could be doing better. My first (real) script is for the credit union I work at, it basically automates logging into a program we use and schedules a job with expect. I had originally written this in regular expect with some tcl but decided to rewrite it in perl.
use strict; use Expect; use POSIX; use Time::ParseDate; use Getopt::Std; my $command = 'connexid'; my $password = 'passwd'; my $login = 'loginid'; my $job = $ARGV[1]; my $queue = $ARGV[2]; my $date = $ARGV[3]; my $time = $ARGV[4]; my $exp; my $when; my %options=(); getopts("hvnlrt", \%options); if ($date) { date(); } 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"; } 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"; } } if ($options{h}) { help(); } if ($options{r}) { if ($date && $time) { repgen(); } elsif ($date) { die "Please specify a time for the job to run. \n"; } else { die "Please specify a date and time for the job to run. \n"; } } if ($options{l}) { if ($date && $time) { later(); } elsif ($date) { die "Please specify a time for the job to run. \n"; } else { die "Please specify a date and time for the job to run. \n"; } } if ($options{n}) { if ($job && $queue) { now(); } else { die "Please specify a job name and a queue. \n"; } } if ($options{v}) { print "autoepisys version 1.3\n"; } sub login{ if ($command eq "local") { $exp = Expect->spawn("sym") or die "Cannot spawn sym $!\n"; $exp->expect(10, '-re', 'UserId'); $exp->send("$login\r"); $exp->expect(10, '-re', 'Dedicate this console to this user?'); $exp->send("y\r"); } else { $exp = Expect->spawn($command) or die "Cannot spawn $command $!\n" +; $exp->expect(10, '-re', 'password:'); $exp->send("$password\r"); $exp->expect(10, '-re', '(#|\$) '); $exp->send("sym\r"); $exp->expect(10, '-re', 'UserId'); $exp->send("$login\r"); $exp->expect(10, '-re', 'Dedicate this console to this user?'); $exp->send("y\r"); } } sub logoff { if ($command eq "local") { $exp->expect(10, '-re', "Do you wish to log off the system?"); $exp->send("y\r"); $exp->soft_close(); } else { $exp->expect(10, '-re', "Do you wish to log off the system?"); $exp->send("y\r"); $exp->expect(10, '-re', '(#|\$) '); $exp->send("exit\r"); $exp->soft_close(); } } sub now { login(); $exp->expect(10, '-re', 'Menu Selection'); $exp->send("8\r"); $exp->expect(10, '-re', 'Menu Selection'); $exp->send("0\r"); $exp->expect(10, '-re', 'Selection'); $exp->send("0\r\r"); $exp->expect(10, '-re', 'Job File Name'); $exp->send("$job\r"); $exp->expect(10, '-re', 'Batch Options?'); $exp->send("Y\r"); $exp->expect(10, '-re', 'Display Batch Queues'); $exp->send("\r"); $exp->expect(10, '-re', 'Notify Upon Completion?'); $exp->send("\r"); $exp->expect(10, '-re', 'Queue Priority'); $exp->send("\r"); $exp->expect(10, '-re', 'Scheduled Start Date'); $exp->send("\r"); $exp->expect(10, '-re', 'Scheduled Start Time'); $exp->send("\r"); $exp->expect(10, '-re', 'Batch Queue'); $exp->send("$queue\r"); $exp->after(10, '-re', 'Exception Item Batch ID'); $exp->send("\r"); $exp->expect(10, '-re', 'Okay?'); $exp->send("Y\r"); $exp->expect(10, '-re', "Batch Job $job Is Done"); $exp->send("\e"); $exp->send("\e"); $exp->send("\e"); logoff(); } sub later { login(); $exp->expect(10, '-re', 'Menu Selection'); $exp->send("8\r"); $exp->expect(10, '-re', 'Menu Selection'); $exp->send("0\r"); $exp->expect(10, '-re', 'Selection'); $exp->send("0\r\r"); $exp->expect(10, '-re', 'Job File Name'); $exp->send("$job\r"); $exp->expect(10, '-re', 'Batch Options?'); $exp->send("Y\r"); $exp->expect(10, '-re', 'Display Batch Queues'); $exp->send("\r"); $exp->expect(10, '-re', 'Notify Upon Completion?'); $exp->send("\r"); $exp->expect(10, '-re', 'Queue Priority'); $exp->send("\r"); $exp->expect(10, '-re', 'Scheduled Start Date'); $exp->send("$when\r"); $exp->expect(10, '-re', 'Scheduled Start Time'); $exp->send("$time\r"); $exp->expect(10, '-re', 'Batch Queue'); $exp->send("$queue\r"); $exp->expect(10, '-re', 'Expected System Date'); $exp->send("$when\r"); $exp->expect(10, '-re', 'Expected Previous System Date'); $exp->send("\r"); $exp->after(10, '-re', 'Exception Item Batch ID'); $exp->send("$when\r"); $exp->expect(10, '-re', 'Okay?'); $exp->send("Y\r"); $exp->send("\e"); $exp->send("\e"); $exp->send("\e"); logoff(); } sub repgen { login(); $exp->expect(10, '-re', 'Menu Selection'); $exp->send("8\r"); $exp->expect(10, '-re', 'Menu Selection'); $exp->send("0\r"); $exp->expect(10, '-re', 'Selection'); $exp->send("1\r\r"); $exp->expect(10, '-re', 'Selection'); $exp->send("11\r"); $exp->expect(10, '-re', 'Specification File'); $exp->send("$job\r"); $exp->expect(10, '-re', 'Selected Run Date'); $exp->send("$when\r"); $exp->expect(10, '-re', "Specification File"); $exp->send("\r"); $exp->expect(10, '-re', "Batch Options?"); $exp->send("Y\r"); $exp->expect(10, '-re', 'Display Batch Queues'); $exp->send("\r"); $exp->expect(10, '-re', 'Notify Upon Completion?'); $exp->send("\r"); $exp->expect(10, '-re', 'Queue Priority'); $exp->send("\r"); $exp->expect(10, '-re', 'Scheduled Start Date'); $exp->send("\r"); $exp->expect(10, '-re', 'Scheduled Start Time'); $exp->send("$time\r"); $exp->expect(10, '-re', 'Batch Queue'); $exp->send("$queue\r"); $exp->expect(10, '-re', 'Okay?'); $exp->send("Y\r"); $exp->send("\e"); $exp->send("\e"); $exp->send("\e"); logoff(); } sub help { print "usage: \[-l\] \[-n\] \[-r\] \[-v\] \[-t\] JOB.NAME queue wh +en time\n"; print " The -l option sets the batch job to run later\n"; print " The -n option runs the batch job now (for script automa +tion)\n"; print " The -r option runs a report generator specification fil +e\n"; print " The -v option shows version information\n"; print " The -t option will show you what will happen with a que +ry\n"; print "Examples:\n"; print " auto -l NETTELLER 3 2days 0130\n"; print " auto -l ACH.MIDNIGHT.POST 2 tomorrow 0030\n"; print " auto -n DRAFT.POST 1\n"; print " auto -r DAILY.REFERENCE.FILE 1 today 1801\n"; print " auto -t SOME.COMMAND 2 3days 1801\n"; exit; } sub date { my $dt = parsedate($date); $when = strftime("%m%d%y", localtime($dt)); chomp($when); }

The code is also here on github: https://github.com/puntme/AutoEpisys/raw/master/epi Thanks in advance.

Comment on code critique
Download Code
Re: code critique
by Old_Gray_Bear (Bishop) on May 27, 2011 at 22:21 UTC
    Take a look at Damian Conway's book 'Perl best Practices' and the attendant Perl module Perl::Critic. The Damian has some good points to make (and a few that I disagree with). Running your code through perl-critic at a reasonable level of strictness (-4 or for the very masochistic -3) can be enlightening.

    Also use warnings in addition to use strict -- suspenders and belt....

    ----
    I Go Back to Sleep, Now.

    OGB

Re: code critique
by JavaFan (Canon) on May 27, 2011 at 22:29 UTC
    My first (real) script is for the credit union I work at

    The code is also here on github:

    And the credit union is ok with you posting their code? Granted, it doesn't do anything really interesting, but many companies aren't ok with people posting their code.
      Thanks. I'll definitely take a look at that module, it looks exactly like what I need. I also appreciate all of your points, this is literally my first real 'program' so I'm sure it sucks.

      And the credit union is ok with you posting their code? Granted, it doesn't do anything really interesting, but many companies aren't ok with people posting their code.

      Its my code, I wrote it with the intention of distributing it to other Credit Unions. This is basically just a fancier version of what I use internally.
Re: code critique
by johngg (Abbot) on May 27, 2011 at 22:48 UTC

    Here are a few suggestions:-

    • I think you should do your options handling before you assign to variables from @ARGV because your options will be in @ARGV until the call to getopts() strips them out.

    • I think your chomp($when); is superfluous because there is no line terminator in $when as you've just created it with a "%m%d%y" template to strftime().

    • Your "help" text could be output by a single print statement with either a comma-separated list of lines or a HEREDOC.

    • Square brackets don't need to be escaped in a double-quoted string. They do in a regular expression if you want to match a literal one as they are regex metacharacters.

    • I've not used Expect but if the -re flag implies that the next argument should be a regex pattern, be aware that a ? is a metacharacter meaning zero or one of the preceding term.

    • You may be able to make your expect/send code more readable by setting up a dispatch table, although some prompts can generate two different responses so the technique can't be applied universally:

      my %dispatchTable = ( ... 'Queue Priority' => "\r", ... ); ... expectSend( 'Queue Priority' ); ... sub expectSend { my $prompt = shift; $exp->expect( 10, '-re', $prompt ); $exp->send( $dispatchTable{ $prompt }; }

    I hope these points are helpful.

    Cheers,

    JohnGG

Re: code critique
by grantm (Parson) on May 28, 2011 at 01:07 UTC

    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? } }
Re: code critique
by Khen1950fx (Canon) on May 28, 2011 at 05:51 UTC
    Here's just a few problems that I saw.

    First, from the beginning, don't forget the shebang line. You can do a short shebang such as #!perl.

    Second, I had to adjust the use statements like this:

    #!/usr/bin/perl use strict; use warnings; use Expect; use Fcntl qw(:DEFAULT :flock); use Getopt::Std; use POSIX qw(strftime); use Time::ParseDate;
    I used Fcntl instead of POSIX because, at least for me, Fcntl gives me greater control than POSIX::fcntl_h; also, it wouldn't work as use Posix;, but it can work as use POSIX qw(:fcntl_h), either way.

    In your last subroutine you used:

    $when = strftime( '%m%d%y', localtime $dt );
    Here perl complained about strftime being an undefined subroutine; hence, there I used use POSIX qw(strftime) to fix that.

    I use the following mantra when I critque myself or others:

    perl -c script.pl perl -MO=Lint script.pl perl -MO=Deparse script.pl perltidy script.pl
    Here's the results:
    #!/usr/bin/perl use strict; use warnings; use Expect; use Fcntl qw(:DEFAULT :flock); use Getopt::Std; use POSIX qw(strftime); use Time::ParseDate; my $command = 'connexid'; my $password = 'passwd'; my $login = 'loginid'; my $job = $ARGV[1]; my $queue = $ARGV[2]; my $date = $ARGV[3]; my $time = $ARGV[4]; my $exp; my $when; my (%options) = (); getopts 'hvnlrt', \%options; if ($date) { date(); } if ( $options{'t'} ) { if ( $job and $queue and $date and $time ) { print "At $time on $when, this will run $job in queue $queue. +\n"; } elsif ( $job and $queue and $date ) { print "Please enter a time for the job to run. \n"; } elsif ( $job and $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"; } } if ( $options{'h'} ) { help(); } if ( $options{'r'} ) { if ( $date and $time ) { repgen(); } elsif ($date) { die "Please specify a time for the job to run. \n"; } else { die "Please specify a date and time for the job to run. \n"; } } if ( $options{'l'} ) { if ( $date and $time ) { later(); } elsif ($date) { die "Please specify a time for the job to run. \n"; } else { die "Please specify a date and time for the job to run. \n"; } } if ( $options{'n'} ) { if ( $job and $queue ) { now(); } else { die "Please specify a job name and a queue. \n"; } } if ( $options{'v'} ) { print "autoepisys version 1.3\n"; } sub login { use strict 'refs'; if ( $command eq 'local' ) { die "Cannot spawn sym $!\n" unless $exp = 'Expect'->spawn('sym +'); $exp->expect( 10, '-re', 'UserId' ); $exp->send("$login\r"); $exp->expect( 10, '-re', 'Dedicate this console to this user?' + ); $exp->send("y\r"); } else { die "Cannot spawn $command $!\n" unless $exp = 'Expect'->spawn($command); $exp->expect( 10, '-re', 'password:' ); $exp->send("$password\r"); $exp->expect( 10, '-re', '(#|\\$) ' ); $exp->send("sym\r"); $exp->expect( 10, '-re', 'UserId' ); $exp->send("$login\r"); $exp->expect( 10, '-re', 'Dedicate this console to this user?' + ); $exp->send("y\r"); } } sub logoff { use strict 'refs'; if ( $command eq 'local' ) { $exp->expect( 10, '-re', 'Do you wish to log off the system?' +); $exp->send("y\r"); $exp->soft_close; } else { $exp->expect( 10, '-re', 'Do you wish to log off the system?' +); $exp->send("y\r"); $exp->expect( 10, '-re', '(#|\\$) ' ); $exp->send("exit\r"); $exp->soft_close; } } sub now { use strict 'refs'; login; $exp->expect( 10, '-re', 'Menu Selection' ); $exp->send("8\r"); $exp->expect( 10, '-re', 'Menu Selection' ); $exp->send("0\r"); $exp->expect( 10, '-re', 'Selection' ); $exp->send("0\r\r"); $exp->expect( 10, '-re', 'Job File Name' ); $exp->send("$job\r"); $exp->expect( 10, '-re', 'Batch Options?' ); $exp->send("Y\r"); $exp->expect( 10, '-re', 'Display Batch Queues' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Notify Upon Completion?' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Queue Priority' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Scheduled Start Date' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Scheduled Start Time' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Batch Queue' ); $exp->send("$queue\r"); $exp->after( 10, '-re', 'Exception Item Batch ID' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Okay?' ); $exp->send("Y\r"); $exp->expect( 10, '-re', "Batch Job $job Is Done" ); $exp->send("\e"); $exp->send("\e"); $exp->send("\e"); logoff; } sub later { use strict 'refs'; login; $exp->expect( 10, '-re', 'Menu Selection' ); $exp->send("8\r"); $exp->expect( 10, '-re', 'Menu Selection' ); $exp->send("0\r"); $exp->expect( 10, '-re', 'Selection' ); $exp->send("0\r\r"); $exp->expect( 10, '-re', 'Job File Name' ); $exp->send("$job\r"); $exp->expect( 10, '-re', 'Batch Options?' ); $exp->send("Y\r"); $exp->expect( 10, '-re', 'Display Batch Queues' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Notify Upon Completion?' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Queue Priority' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Scheduled Start Date' ); $exp->send("$when\r"); $exp->expect( 10, '-re', 'Scheduled Start Time' ); $exp->send("$time\r"); $exp->expect( 10, '-re', 'Batch Queue' ); $exp->send("$queue\r"); $exp->expect( 10, '-re', 'Expected System Date' ); $exp->send("$when\r"); $exp->expect( 10, '-re', 'Expected Previous System Date' ); $exp->send("\r"); $exp->after( 10, '-re', 'Exception Item Batch ID' ); $exp->send("$when\r"); $exp->expect( 10, '-re', 'Okay?' ); $exp->send("Y\r"); $exp->send("\e"); $exp->send("\e"); $exp->send("\e"); logoff; } sub repgen { use strict 'refs'; login; $exp->expect( 10, '-re', 'Menu Selection' ); $exp->send("8\r"); $exp->expect( 10, '-re', 'Menu Selection' ); $exp->send("0\r"); $exp->expect( 10, '-re', 'Selection' ); $exp->send("1\r\r"); $exp->expect( 10, '-re', 'Selection' ); $exp->send("11\r"); $exp->expect( 10, '-re', 'Specification File' ); $exp->send("$job\r"); $exp->expect( 10, '-re', 'Selected Run Date' ); $exp->send("$when\r"); $exp->expect( 10, '-re', 'Specification File' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Batch Options?' ); $exp->send("Y\r"); $exp->expect( 10, '-re', 'Display Batch Queues' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Notify Upon Completion?' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Queue Priority' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Scheduled Start Date' ); $exp->send("\r"); $exp->expect( 10, '-re', 'Scheduled Start Time' ); $exp->send("$time\r"); $exp->expect( 10, '-re', 'Batch Queue' ); $exp->send("$queue\r"); $exp->expect( 10, '-re', 'Okay?' ); $exp->send("Y\r"); $exp->send("\e"); $exp->send("\e"); $exp->send("\e"); logoff(); } sub help { use strict 'refs'; print "usage: [-l] [-n] [-r] [-v] [-t] JOB.NAME queue when time\n" +; print " The -l option sets the batch job to run later\n"; print " The -n option runs the batch job now (for script automa +tion)\n"; print " The -r option runs a report generator specification fil +e\n"; print " The -v option shows version information\n"; print " The -t option will show you what will happen with a que +ry\n"; print "Examples:\n"; print " auto -l NETTELLER 3 2days 0130\n"; print " auto -l ACH.MIDNIGHT.POST 2 tomorrow 0030\n"; print " auto -n DRAFT.POST 1\n"; print " auto -r DAILY.REFERENCE.FILE 1 today 1801\n"; print " auto -t SOME.COMMAND 2 3days 1801\n"; exit; } sub date { use strict 'refs'; my $dt = parsedate($date); $when = strftime( '%m%d%y', localtime $dt ); chomp $when; }
Re: code critique
by graff (Chancellor) on May 28, 2011 at 17:16 UTC
    Your long sequences of $exp->expect...; $exp->send... are painful. The suggestion above about a dispatch table might be good, or perhaps you could just set up arrays for each subroutine. Something like this would make it a lot easier to see what's being done, and would make maintenance easier too. Here's an example based on the "now" sub:
    sub now { login(); my @comseq = ( [ 'Menu Selection', "8\r" ], [ 'Menu Selection', "0\r" ], [ 'Selection', "0\r\r" ], [ 'Job File Name', "$job\r" ], [ 'Batch Options?', "Y\r" ], [ 'Display Batch Queues', "\r", [ 'Notify Upon Completion?', "\r" ], [ 'Queue Priority', "\r" ], [ 'Scheduled Start Date', "\r" ], [ 'Scheduled Start Time', "\r" ], [ 'Batch Queue', "$queue\r" ], [ 'Exception Item Batch ID', "\r" ], [ 'Okay?', "Y\r" ], [ "Batch Job $job Is Done", "\e" ], ); for my $com ( @comseq ) { $exp->expect( 10, '-re', $$com[0] ); $exp->send( $$com[1] ); } $exp->send("\e"); $exp->send("\e"); logoff(); }
    That said, I would also want to follow the other suggestions about having the subroutines take explicit args and return explicit values -- e.g.:
    sub now { my $exp = login( @_ ); # caller passes login args to us. ... logoff( $exp );

    UPDATE: BTW, if/when you need to use things other than 10 and '-re' as the first and second args on some "expect()" calls, just add those into the @comseq array -- e.g.:

    my @comseq = ( [ 256, '-foo', 'Bar?', "baz\r" ], [ 10, '-re', 'Yes or no?', "Can't decide\r" ], ... ); for my $com ( @comseq ) { my $send = pop @$com; $exp->expect( @$com ); $exp->send( $send ); }

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others meditating upon the Monastery: (14)
As of 2014-08-22 13:30 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    The best computer themed movie is:











    Results (158 votes), past polls