|Pathologically Eclectic Rubbish Lister|
Re^2: RFC: beginner level script improvement (various comments)by smls (Friar)
|on Oct 30, 2013 at 10:26 UTC||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:
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).
One possibility would be to use %CONFIG right from the start:
calling 'next'/'last' to break out of a callback sub...
...is not a good idea.
In process_device you do:
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:
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.)
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.
Or, even better, only turn off specific warning categories instead of all at once - for example:
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:
referencing + dereferencing
I see some contructs like this in your code:
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.
creating temporary files
In populate_config_hash you manually generate a hopefully-unique temporary file name like this:
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:
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:
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")