Re: New Quiz: "What does this code do?" (1-December-2006)

David Landgren <[email protected]> Wed, 20 Dec 2006 21:46:41 +0100
Newsgroups gmane.comp.lang.perl.qotw.discuss
Organization The Lusty Decadent Delights of Imperial Pompeii
Message-ID <[email protected]>
Shlomi Fish did write:
> Hi!
> 
> On Thursday 14 December 2006 00:05, Garrett, Philip (MAN-Corporate) wrote:
>> Owen wrote:
>>> On Fri, 1 Dec 2006 16:35:44 +0200
>>>
>>> Shlomi Fish <shlomif-ik1l9ssToec+JF/[email protected]> wrote:
>>> 1:   my (@list) =
>>> 2:   (
>>> 3:       grep { $cgi->param("$prefix$_") }
>>> 4:       map { /^${prefix}(\d+)$/ ? ($1) : () }
>>> 5:       $cgi->param()
>>> 6:   );
>> Whenever I see construct like this that confuses me, I rewrite it so
>> that the
>> code actually goes in the same order as the operations, and turn all the
>> temporary lists into named arrays.  If the resulting code is clearer and
>> does
>> not negatively affect performance in a substantial way, I'll leave it.
>>
>>     my $prefix = "form_field_";
>>
>>     # line 5
>>     my @param_names = $cgi->param;
>>
>>     # line 4
>>     my @received_fields;
>>     for my $param_name (@param_names) {
>>         if ($param_name =~ /^${prefix}(\d+)$/) {
>>             my $field_number = $1;
>>             push @received_fields, $field_number;
>>         }
>>     }
>>
>>     # line 3
>>     my @list;
>>     for my $field_number (@received_fields) {
>>         if ($cgi->param("${prefix}${field_number}")) {
>>             push @list, $field_number;
>>         }
>>     }
>>
>>     # @list now contains a list of numbers for which there are
>>     # populated form fields named "form_field_<number>".
>>     # ** it might contain duplicates, too
>>     # ** "populated" means non-empty and non-zero
>>
> 
> Wow-wa! You took a 3-liner and turned it into a 10-15 lines verbose 
> monstrosity. If you're experienced enough you'll find the more concise map 
> and grep version, as easy to understand as your code if not more. (Because 
> there's less to read).

I wouldn't call it a monstrosity. It may be verbose, but the code does 
have the advantage that you can stick a breakpoint in the debugger and 
stop it exactly where you want to. Sometimes that's important too. The 
question is finding the right balance.

I'm dealing with a pile of crappy nested map/map/grep/for/map/grep 
chains in some code that a consultant wrote for me a few months ago and 
it's a royal pain.

David
-- 
"It's overkill of course, but you can never have too much overkill."