Steve Bertrand wrote:
Hi everyone,
Hello,
I'm reviewing a script I wrote that I use against one of my modules. The
script takes an optional param at the command line.
Although I am seriously reviewing my options for better and smarter ways
to utilize command line args, I'm curious as to why perlcritic complains
to me, and (in this specific case) how it should have been done differently.
Without getting into the different ways to accept and process command
line args, will someone point out how _this_ script should have been
written to avoid the critic message? Is leaving off the condition the
same as having it there?
I've left what perlcritic yelled about, and the script in it's entirety.
The offending line is marked:
acct-dev: ISP-RADIUS % perlcritic src/utilities/aggregate_monthly
Variable declared in conditional statement at line 21, column 1.
Declare variables outside of the condition. (Severity: 5)
#!/usr/bin/perl
# - aggregate_monthly
# - utility script for ISP::RADIUS
# - aggregates totals from aggregate_daily db table into
# the aggregate_monthly db table
# - same license as module itself
# If a month is passed in as the first parameter in the format
# YYYY-MM, we will operate on that month. Otherwise, the month
# that was existent yesterday will be used.
use strict;
use warnings;
use DateTime;
use ISP::RADIUS;
my $radius = ISP::RADIUS->new();
# issue is down ---vvvvvvvvvvv
my $month = $ARGV[0] if $ARGV[0];
perldoc perlsyn
[ SNIP ]
NOTE: The behaviour of a "my" statement modified with a statement
modifier conditional or loop construct (e.g. "my $x if ...") is
undefined. The value of the "my" variable may be "undef", any
previously assigned value, or possibly anything else. Don’t rely on
it. Future versions of perl might do something different from the
version of perl you try it out on. Here be dragons.
if ( $month !~ m{ \A \d{4}-\d{2} \z }xms ) {
print "\nInvalid date parameter. Must be supplied as 'YYYY-MM'\n\n";
exit;
You exit the program if $month is not equal to a seven character string.
}
if ( $month ) {
A seven character string is *always* true.
$radius->aggregate_monthly( { month => $month } );
}
else {
So this branch will *never* execute, and the whole test is superfluous.
$radius->aggregate_monthly();
}
Probably better as:
if ( $month =~ /\A\d{4}-\d{2}\z/ ) {
$radius->aggregate_monthly( { month => $month } );
}
else {
print "\nInvalid date parameter. Must be supplied as 'YYYY-MM'\n\n";
exit 1; # non-zero exit lets the OS know an error occured
}
John
--
The programmer is fighting against the two most
destructive forces in the universe: entropy and
human stupidity. -- Damian Conway
--
To unsubscribe, e-mail: beginners-unsubscr...@perl.org
For additional commands, e-mail: beginners-h...@perl.org
http://learn.perl.org/
