Uri Guttman wrote: >>>>>> "SB" == Steve Bertrand <[email protected]> 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
smime.p7s
Description: S/MIME Cryptographic Signature
