Beefy Boxes and Bandwidth Generously Provided by pair Networks
more useful options
 
PerlMonks  

comment on

( [id://3333]=superdoc: print w/replies, xml ) Need Help??

Some random comments on your code follow. Please keep in mind that I only casually perused (parts of) your script, so don't consider this a proper code review by any means. I also don't really understand much of the underlying functionality, so this is restricted to Perl-specific aspects of the code.

storing config options in a list of scalars vs. a hash

In process_command_line_args you initialize a long list of variables, and pass them back as an array reference:

sub process_command_line_args { #set default values for dynamic config settings my ($layout,$wait_for_child,$wait_for_exec,$prompt,$configf,$quiet +,$outf,$debug,$batchsize,$proto,$tacacs,$verbose,$header,@maildst) = +(0,120,3,1,"$name.cfg",1,"$name.csv",0,0,"none",0,0); GetOptions ( ...) ... my @args = ($layout,$wait_for_child,$wait_for_exec,$name,$prompt,$ +configf,$quiet,$outf,$debug,$batchsize,$proto,$tacacs,$verbose,$heade +r,$mailref,$path); return \@args; }

Then in various subs (Main, validate_command_line_args, populate_config_hash) you access its elements via array subscripts and by repeatedly unpacking it into a long list of scalars again.

This seems like a really cumbersome way to deal with those values (making your script more difficult to maintain).
A hash would serve them much better I think, especially since in populate_config_hash you end up creating the %CONFIG hash from them anyways.

One possibility would be to use %CONFIG right from the start:

sub Main { my %CONFIG; initialize_config_hash(\%CONFIG); get_command_line_args(\%CONFIG); validate_command_line_args(\%CONFIG); get_username_and_password_from_stdin(\%CONFIG) if $CONFIG{prompt}; ... } sub initialize_config_hash { my $CONFIG = shift; %$CONFIG = ( quiet => 0 , cfg => "$name.cfg" , verbose => 0 , outcsv => "$name.csv" , debug => 0 , proto => "none" , prompt => 1 , header => undef , layout => 0 , maildst => [] , wait_for_child => 120 , mail_to => "" , wait_for_exec => 0 , mail_from => "NMS-script@me.net" , batchsize => 0 , mail_sub => "" , tacacs => 0 , mail_cc => "" , tacuser => "user" , mail_data => "" , tacpass => "pass" , ); } sub get_command_line_args { my $CONFIG = shift; GetOptions ( 'layout|l' => \$CONFIG->{layout}, 'file-based-auth|f' => sub { \$CONFIG->{prompt} = 0; }, 'prompt-based-auth|p' => sub { \$CONFIG->{prompt} = 1; }, ... ) ... }

calling 'next'/'last' to break out of a callback sub...

...is not a good idea.

In process_device you do:

$WARN = 0; #supress flow control warning in following block due to exi +ting anon sub LOOPCTRL: while ($lb < 10){ $exp->expect(... [ ..., sub {... $lb++; next LOOPCTRL; }], ... ); $exp->expect(... [ ..., sub {... last LOOPCTRL; }] ); } $WARN = 1; # reset to default

The warning is there for a reason: The callback now never returns to the code that called it (in this case, Expect.pm), so if that code needs to do any cleanup etc. before passing control back to you, that will be skipped which might lead to hard-to-track bugs.

I don't have hands-on experience with the Expect.pm module, but the documentation suggests that expect() returns a positive number on success and undef otherwise, so it seems to me that this loop could be re-written like so:

while (...) { next if $exp->expect(... ... ); last if $exp->expect(... ... ); }

Suppressing the warning becomes unnecessary, and so is the "LOOPCTRL" label btw.

(Also make extra sure that those expect calls + loop control really result in the intended behavior... I can't help you there, because I have no understading of the external program you're controlling.)

suppressing warnings

In my experience, suppressing core Perl warnings is rarely the best solution - usually they are an indicator that what you're trying to do can be done in a better/safer way.
But even when you do want to temporarily suppress Perl warnings, using a global variable together with a global signal handler is not the proper way to do it. Instead, simply disable the "warnings" pragma within an as-small-as-possible lexical scope:

use strict; use warnings; # warnings coming from here will be printed { no warnings; # warnings coming from here will NOT be printed } # warnings coming from here will be printed

Or, even better, only turn off specific warning categories instead of all at once - for example:

# Don't warn about exiting subs via loop control: no warnings 'exiting'; # Don't warn about using undef in numeric or string operations: no warnings 'uninitialized';

To suppress warnings emitted by modules (rather than Perl itself), first look if the module provides an option to turn them off. If not, overriding the warn signal handler is the way to go, but this too is better done locally instead of for the whole program:

{ local $SIG{__WARN__} = sub {}; # call the noisy module function here } # warnings coming from here will not be affected

referencing + dereferencing

I see some contructs like this in your code:

\%{ $CONFIG } \%$CONFIG

You can just write $CONFIG instead. The % dereferences the hash ref, but then the \ takes the reference again - i.e. they cancel each other out.
Of course if you're doing it on purpose, to remind yourself that what is being passed along is a hash ref - then don't let me keep you from doing so... :)

creating temporary files

In populate_config_hash you manually generate a hopefully-unique temporary file name like this:

my $range = '1000000'; my $rand = int(rand($range)); ##generate random temp-db filename to al +low for simultanous script execution ... tempdb => "$path/$rand.db",

There is a module for safely creating temporary files: File::Temp (which is part of Perl since v5.6.1)

unpacking a subroutine argument

At the beginning of subroutines that only take one argument, you tend to write:

my $foo = $_[0];

There's nothing wrong with this technically, but I thought I'd point out that the following idioms are much more commonly used for that:

my $foo = shift;
my ($foo) = @_;

Using shift for this may seem particularly strange to those new to Perl, but it has a lot of tradition and is also very fast.

Edit: Fixed typo ("shift" not "unshift")


In reply to Re^2: RFC: beginner level script improvement (various comments) by smls
in thread RFC: beginner level script improvement by georgecarlin

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



  • Are you posting in the right place? Check out Where do I post X? to know for sure.
  • Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
    <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
  • Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
  • Want more info? How to link or How to display code and escape characters are good places to start.
Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Chatterbox?
and the web crawler heard nothing...

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

    No recent polls found