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!)
my $scan = 'foo';
# here $scan == 'foo';
if ( $something ) {
my $scan = 'bar';
# here $scan == 'bar';
}
# now the $scan we defined in the if() {} is out of scope,
# gone, defunct, forgotten, inaccessible so $scan == 'foo'
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:
print "
=======================================
Here is my really groovy script
I hope you like it
Call it with --help to get the synopsis
=======================================
";
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.
With simple scripts you can parse @ARGV yourself. For complex command line syntax Getopt::Std and Getopt::Long are the standards (I don't like them much myself but that is another story....)
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:
print "Oops";
die;
# might as well be
die "Oops";
When you open a file you should check that the call worked like so:
open FILE, $somefile or die "Could not read $somefile, perl says the e
+rror is $!\n";
# note if you want to use || rather than or you need to be aware
# that it binds tighter than the , operator so you need parnes thusly
open (FILE, ">$somefile") || die "Could not write $somefile, $!\n";
Almost lastly you can just do:
while ( my $line=<FILE> ) { blah }
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:
volume ;my_hacking_script_that_will_now_get_executed.pl
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:
system(@stuff)
check_call($custom_message,@stuff);
sub check_call {
my $custom_message = shift;
my @call = @_;
# check for 0 return status
return unless $?;
# bugger, we have an error so tell all
# Work out return code
my $rc = $? >> 8;
die qq!
System call failed
Error code $rc
$custom_message
Actual call was
system( "@call" )!;
}
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.
use Carp;
one();
sub one { two() }
sub two { three() }
sub three{ confess "It took a while to get here, but I know just how I
+ did!" }
__DATA__
It took a while to get here, but I know just how I did! at script line
+ 7
main::three() called at script line 6
main::two() called at script line 5
main::one() called at script line 3
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:
#!/usr/bin/perl -w
use strict;
use Error::Custom;
synopsis() unless @ARGV;
system(@args);
check_call('custom message',@args); # this lives in Error::Custom
system(@args1);
check_call('more custom messages',@args1);
exit;
sub synopsis {
# note that $0 contains the script name so the name will
# always be the correct one no matter what you call the script
print "
Synopsis $0 SOME_FLAG SOME_ARG
blah
blah\n\n";
exit;
}
The module would look like:
package Error::Custom;
use strict;
use Exporter;
use Carp;
use vars qw($VERSION @ISA @EXPORT @EXPORT_OK );
$VERSION = 1.00;
@ISA = qw(Exporter);
@EXPORT = qw(check_call);
@EXPORT_OK = qw(check_call);
sub check_call {
my $custom_message = shift;
my @call = @_;
# check for 0 return status
return unless $?;
# bugger, we have an error so tell all
# Work out return code
my $rc = $? >> 8;
confess qq!
System call failed
Error code $rc
$custom_message
Actual call was
system( "@call" )
!;
}
1;
cheers
tachyon
s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print
| 