John W. Krahn wrote:
> 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.
ehhh.. *scratching head while thinking about dragons*.
Is my regex at least doing what you perceive it should be doing? It
tests ok here:
% perl -e '$var="aaaa-bb"; print 1 if $var !~ m{ \A \d{4}-\d{2} \z }xms'
I'm so confused :P
> 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.
...but it works for real against the real database, and I get proper
results (with my original code in the OP posting).
> 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
> }
...I think I get what you are saying, and I think I found that out in
the last message I sent.
However, I still don't understand why you claim that my original regex
was looking for a "seven character string", when I *thought* I was
checking for four digits, then two digits separated by a dash (or
hyphen, as it were).
Thanks John,
Steve
--
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
http://learn.perl.org/
