Webmaster Forum / Perl / Getting Started / July 2009

 SB> Uri Guttman wrote:
 >>
 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.

 SB> Thanks Uri,

 SB> I get the gist of what you are doing here. My example 'item_name' was a
 SB> bad one ;)

 SB> 'item_name' is literally a key in a hash where the value is the name of
 SB> a purchased item. It is extracted from a hash that has ~10 other
 SB> purchase-type items (comment, amount etc) ;)

 SB> Not every key in the incoming %$data parameter has the same value type,
 SB> so AFAIK, I have to check each item in the hash individually as opposed
 SB> to just assigning it to a temporary var. (...I'm working away from a
 SB> predecessor's method of using $tmp, $a, $b, $c, $left, $right and
 SB> everything else, so I don't do things like that ;)

 SB> Common-code doesn't really bother me, so long as I'm writing it in a
 SB> context where it needs to be repeated for different data types, and once
 SB> it's done, I can re-use it. Shrinking the repeating code blocks is a
 SB> different story.

 SB> I'm learning new tricks far faster than I can code. If I can get 100
 SB> lines of great working code done today, then while I'm reading tonight,
 SB> I'll learn new ways on how to write that code ;)

 SB> Thanks Uri,

 SB> Steve

 SB> ps. The module in question is here: http://ipv6canada.com/Sanity.pm
 SB> pps. I'm currently reading "Advanced Perl Programming" by Sriram
 SB> Srinivasan. (Yes, I know there's a new version).

 SB> Steve

 SB> Uri Guttman wrote:

 >> 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.

 SB> I get the gist of what you are doing here. My example 'item_name' was a
 SB> bad one ;)

item_name is an ok name if the term item is clear enough. in a proper
context it works. item by itself without context is too generic. so is
name. also it depends on scoping. key and val are poor names but if you
are doing a tight loop over hash keys, they are fine.

 SB> Not every key in the incoming %$data parameter has the same value
 SB> type, so AFAIK, I have to check each item in the hash individually
 SB> as opposed to just assigning it to a temporary var. (...I'm
 SB> working away from a predecessor's method of using $tmp, $a, $b,
 SB> $c, $left, $right and everything else, so I don't do things like
 SB> that ;)

then you should use a validation module or write a sub to do it
yourself. doing validation inline for each field is tedious and
repetitive.

 SB> Common-code doesn't really bother me, so long as I'm writing it in a
 SB> context where it needs to be repeated for different data types, and once
 SB> it's done, I can re-use it. Shrinking the repeating code blocks is a
 SB> different story.

common code should bother you. it is a source of bugs (changed the code
in one place but not the other!), it can slow things down if the code
does repeated slow deep hash accesses, etc. consider the whole concept
of a sub was to factor out duplicated common code into a single
location.

 SB> ps. The module in question is here: http://ipv6canada.com/Sanity.pm
 SB> pps. I'm currently reading "Advanced Perl Programming" by Sriram
 SB> Srinivasan. (Yes, I know there's a new version).

and the two versions are basically very different books. there is almost
no overlap. the first one is also obsolete in many areas or covered
better by new books and docs.

uri