|Perl Monk, Perl Meditation
Re: First Unix Admin Script - Request for Constructive Critisismby tachyon (Chancellor)
|on Feb 08, 2003 at 16:18 UTC
Well here is a list that springs to mind:
You are not using warnings or strict, see use strict, warnings and diagnostics or die You don't need them for trivial scripts but once you add a few more lines they will save you from all manner of trivial errors that will otherwise make life miserable.
Although you are not using strict you have a single my statement which forms a bug (it would be fine if you did had been consistent!)
In your code by localising $scan within the if and then later trying to access it you have ensures that the -s option can never be run as $scan will always be undefined. strict and warnings would have helped you avoid this bug.
You use somelongvarname and other_long_varname which irks me because it is inconsistent. Either use one or the other or evenDoTheCamelHumptyDumpty but do it all the one way and you will not have to go ??? did I use _ in that var_name or not???
When you want to print a bunch of literal stuff it is easier on the eye and fingers to do stuff like:
Note we are just using literal tabs and newlines.
Speaking of a synopsis it is useful to include one so that if the script is called with no args (if it needs them) or a ?, -h, --help or unknown argument you spew it out.
Without strict you can call a function like some_func but this is called a 'bare word' as perl has to make the assumption that this represents a function call. Typically you will call functions like some_func() if it takes no args or some_func(@ARGS) if it does take args. You can also call subs like &some_func or &some_func() which have some very subtle differences that won't worry you so I will just mention that they exist.
You are checking the return codes from your system calls which is good. You don't need to print($string) and then die. You can simply die $string which will print the string to STDERR and then die in the one line:
When you open a file you should check that the call worked like so:
Almost lastly you can just do:
The defined() is implicit in the while loop.
Second lastly you can/should use the multi arg version of system rather than interpolating into a string as this will escape shell meta chars for you automagically and prevents hacking by editing a file so that a line contains something like:
If I inserted this line into mountpointdata then your system call would exec my script (probably as root ;-) This is because the string interpolation occurs before the system call. If you use multiarg system this won't work as ;script.pl is not a valid arg.
Lastly you have a lot of recurrent code after your system calls so why not write a sub so you can do:
As your scripts get more complex you will find that the Carp module comes in handy as it give you a stack trace which can ofen be a lot more useful than just dying in a sub as it tells you how the sub got called. I would suggest using it for the example above as it will then add details of where in the script the error handler was called.
If you are going to be doing a lot of system type scripts I would just put this sub in a module say called Error::Custom (see Simple Module Tutorial then How to Make a CPAN Module Distribution and finally A Guide to Intalling Modules so you can just go:
The module would look like: