Slash Boxes
NOTE: use Perl; is on undef hiatus. You can read content, but you can't post it. More info will be forthcoming forthcomingly.

All the Perl that's Practical to Extract and Report

use Perl Log In

Log In

[ Create a new account ]

Ovid (2709)

  (email not shown publicly)
AOL IM: ovidperl (Add Buddy, Send Message)

Stuff with the Perl Foundation. A couple of patches in the Perl core. A few CPAN modules. That about sums it up.

Journal of Ovid (2709)

Monday October 11, 2004
03:11 PM

More 'iffy' rants

[ #21296 ]

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

The Fine Print: The following comments are owned by whoever posted them. We are not responsible for them in any way.
More | Login | Reply
Loading... please wait.
  • 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.

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

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