More 'iffy' rants

Ovid on 2004-10-11T20:11:23

The following code has a bug:

sub _select_fields
{
    my $self = shift;
    my @fields;
                                                                                                                 
    if ($self->request->param('rev_type') eq 'both') {
        push @fields, ('rev', 'theater_avg');
    } elsif ($self->request->param('rev_type') eq 'total') {
        push @fields, 'rev';
    } else {
        push @fields, 'theater_avg';
    }
                                                                                                                 
    push @fields, 'num_theaters', 'day_multiple';
    return @fields;
}

It turns out that it's supposed to default to "both" instead of its current invisible "loc_avg" default. Further, a new feature has been requested: "day_multiple" should only show up if specifically requested. I certainly didn't want to add to this nasty if/else chain, so I made it a hash. Now it's nice and easy to read.

sub _select_fields
{
    my $self = shift;
    my %rev_types = (
        both    => [ qw/rev  theater_avg  num_theaters/ ],
        total   => [ qw/rev               num_theaters/ ],
        loc_avg => [ qw/     theater_avg  num_theaters/ ],
        all     => [ qw/rev  theater_avg  num_theaters  day_multiple/ ],
    );
                                                                                                                 
    my $rev_type = $self->request->param('rev_type');
    $rev_type    = 'both' unless exists $rev_types{$rev_type};
    return @{$rev_types{$rev_type}};
}

A boolean "if" isn't such a bad thing. It's the nasty if/elsif/else chains that keep cropping up that are problematic. Also note that we've added functionality but the code is shorter and easier to read.

Remember kids: friends don't left friends use "if."

premature optimisation?

nicholas on 2004-10-11T20:22:43

Personally I'd put the initialisation of %rev_types outside the subroutine (but inside anther block to limit the scope) to avoid initialising it each time through the subroutine. But this may well smack of premature optimisation. Then again, it makes it clear that it's a constant structure.

Re:premature optimisation?

Ovid on 2004-10-11T20:33:37

No, you're right. I should "use constant ..." to make it clear what's going on. Whenever I do a quick refactoring in place, I try to limit the changes. Sometimes that means I wind up not changing enough.

Re:premature optimisation?

jplindstrom on 2004-10-11T20:55:40

I usually do that too.

But beware. Sometimes that practise bites me when I eventually implement a class, not in it's own .pm file like I usually do, but just by switching package in the .pl file. So the variable initialization doesn't happen until afterwards and I end up with an empty data structure.

And there is much confusion.

Re:premature optimisation?

Aristotle on 2004-10-12T01:09:19

That's why you should do declaration and initialization in separate steps, and wrap the initialization with a BEGIN.

{
    my %dispatch;
    BEGIN {
         %dispatch = (
             foo => sub { ... },
             bar => sub { ... },
             baz => sub { ... },
         );
    }

    sub quux {
        # ...
    }
}