Uri Guttman wrote:
>>>>>> "SB" == Steve Bertrand <steve@ibctech.ca> writes:
>
> SB> The problem I have, is that I don't like the fact that the "if"
> SB> condition contains the exact same line of code that a sub-section of the
> SB> add_message() function is receiving as a parameter. I know there is a
> SB> way to bundle it better, but in my testing, I haven't been able to do it.
>
> use some temp vars (NOT named temp) to factor out the common code.
>
> my $item_name = $data->{item_name} ;
>
> SB> if (length($data->{item_name}) == 0) {
>
> unless ( length( $item_name ) ) {
>
> length will be false if it is 0 so you don't need to check against 0.
>
> and if you don't allow '' or '0' you can drop length as well.
>
> SB> $error->add_message( "item_name is undefined" );
> SB> }
> SB> if ($self->safe_string($data->{item_name})) {
>
> if( my $safe_name = $self->safe_string($item_name) ) {
>
> SB> $error->add_message( "item_name has potentially dangerous chars:".
> SB> $self->safe_string($data->{item_name})
>
> $error->add_message(
> "item_name has potentially dangerous chars: $safe_name" )
>
> you can also use statement modifiers for the if blocks now that the code
> is shorter
>
> so it looks like this now:
>
> my $item_name = $data->{item_name} ;
> $error->add_message( "item_name is undefined" )
> unless length $item_name ;
>
> my $safe_name = $self->safe_string($item_name) ) {
> $error->add_message(
> "item_name has potentially dangerous chars: $safe_name" )
> if $safe_name ;
>
> that is shorter, faster (no blocks, no duplicate code), and easier to
> read in general.
Thanks Uri,
I get the gist of what you are doing here. My example 'item_name' was a
bad one ;)
'item_name' is literally a key in a hash where the value is the name of
a purchased item. It is extracted from a hash that has ~10 other
purchase-type items (comment, amount etc) ;)
Not every key in the incoming %$data parameter has the same value type,
so AFAIK, I have to check each item in the hash individually as opposed
to just assigning it to a temporary var. (...I'm working away from a
predecessor's method of using $tmp, $a, $b, $c, $left, $right and
everything else, so I don't do things like that ;)
Common-code doesn't really bother me, so long as I'm writing it in a
context where it needs to be repeated for different data types, and once
it's done, I can re-use it. Shrinking the repeating code blocks is a
different story.
I'm learning new tricks far faster than I can code. If I can get 100
lines of great working code done today, then while I'm reading tonight,
I'll learn new ways on how to write that code ;)
Thanks Uri,
Steve
ps. The module in question is here: http://ipv6canada.com/Sanity.pm
pps. I'm currently reading "Advanced Perl Programming" by Sriram
Srinivasan. (Yes, I know there's a new version).
Steve
|