Perl Angst

ziggy on 2003-08-02T21:49:12

Tim Bray published a piece on Perl Angst, where he comments on this code he wrote recently:

Perl is such a great language, except for when it’s not. There’s this problem, which is best illustrated by example: a colleague came into my office with a testy expression and said “You wrote this; what the $#@!%! does it do?!?” I told him I’d get right back to him.

@fields = (0, @fields);
while ($r = $q->fetchrow_arrayref)
{   
  my $topic = $r->[0];
  while ($topic != -1) 
  {   
    my $i; 
    foreach $i (1 .. $#{$r})
    {   
      $totals{$fields[$i]}->[$topic] += $r->[$i];
    }   
    $topic = $parents[$topic];
  }   
}   
shift @fields;

Tim commented that he got some feedback from the usual suspects chiding him for not commenting his code.

I don't think Tim's code needs to be commented. I think Tim needs to use clearer idioms. It's great that he was able to write something like this quickly and get his job done. It's great that it has really good performance. It sucks that it's a using a bunch of mixed metaphors that make it difficult to understand five minutes later.

Here's what I sent to Tim. It's also uncommented, but I think it states his intention much more clearly. It uses a smaller number of idioms and uses them consistently.

while ($r = $q->fetchrow_hashref)
{   
  my $topic = $r->{id};
  
  while ($topic != -1) 
  {   
    $totals{$_}->[$topic] += $r->{$_} for @fields;
    $topic = $parents[$topic];
  }   
}

To be fair, I made some assumptions on his data structures, particularly the content of @fields. And I kept Tim's style for ease of comparison. This same block of code in my own style would be a smidgeon shorter.

---

nice

clscott on 2003-08-03T00:54:33

He mentioned that he was worried about speed so the suggestion I sent him I kept the call to fetchrow_arrayref
since it is still marginally faster than fetchrow_hashref:

while ($r = $q->fetchrow_arrayref)
{
  my $topic = shift @$r;
  while ($topic != -1)
  {
    for ( my $i; $i < @$r ; $i++ ){
    {
      $totals{$fields[$i]}->[$topic] += $r->[$i];
    }
    $topic = $parents[$topic];
  }
}

But, I think he did need a comment along the lines of "Calculate totals for all topics in each field".
Then the other programmer wouldn't have had to ask "What does this do?"
He would know what it was supposed to do and could then figure out how the code did it.

Comments, schmomments.

schwern on 2003-08-04T00:31:13

Forget about comments, that's a subroutine if I ever saw one.

Another version

lachoy on 2003-08-03T02:47:41

I did the same on my site but interspersed it with comments about why I made the changes and gave variable naming the attention it (IMO) deserves for something running three times a day...

Everyone's doing it

pdcawley on 2003-08-03T06:37:21

I think Tim's point that he wasn't sure how to comment the thing is valid. My suggestion (which bounced 'cos I wasn't sure of Tim's email address) was to pull the inner loop up into its own function, which gave an opportunity to name what it does (I called it increment_totals_for_hierarchy, it's not a great name, but it does get some of the flavour of what's happening).

I might also be tempted to rejig the %totals datastructure so that the while ($sth->fetchrow_arrayref) just added numbers for single nodes, then write a recalculate_all_totals function which could run without holding a lock on the database.

And maybe I've been looking at smalltalk for too long, but wouldn't it be good if you could do:

$sth->foreach_hashrow(sub {
    my $topic = $_->{topic};
    ...
});

or, in Perl 6:

foreach_hashrow $sth: -> $row {
    my $topic = $row.{topic};
    while ($topic != -1) {
        %totals{%totals.keys}[$topic] >>+=<< %row{%totals.keys}
    }
    $topic = %parents{$topic};
}

Maybe the semantics of >>+= will mean you don't have to make the hash slices, but I have the feeling you will...

Re:Everyone's doing it

pdcawley on 2003-08-03T06:39:45

Gah, got the sigils wrong on the Perl 6 example. Just do s/\$row/%row/ and all shall be well. (I think s/$row/%row/ should work too mind...

Just kill me okay?

pdcawley on 2003-08-03T06:40:23

Kill me now. I'm too dumb to live.