The first objection I can see to your "use a hash!" advice is something like "Oh, but there's way too much code inside the if/elsif/else blocks; no way can I do everything with a hash lookup!" That just doesn't hold. If the spec's so malleable that you're hacking up your if blocks and adding tons of special cases, you should probably be using a dispatch table instead.

The if construct can be soaked up in a trinary operator, too. Here's another way to organise things:

{
    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/],
    };

    sub _rev_types {
        my $key = @_ ? shift : 'both';
        exists $rev_type->{$key}?
          @{$rev_types->{$key}} :
          @{$rev_types->{'both'}};
    }
}

sub _select_fields {
    my $self = shift;
    return _rev_types( $self->request->param('rev_type'));
}
[download]

This doesn't work when the expressions in the if/elsif/else are complicated. For example, if the second else had to handle the undef param case. Or there was a case for handling ranges or multiple possibilities with the same code.

That can still be easily solved with a dispatch table.

    sub _select_fields
    {
        my $self = shift;
        my %rev_types = (
            both    => sub { 
                # some difficult logic
            },
            total   => sub { [qw/foo bar/] },
        );

        my $rev_type = $self->request->param('rev_type');
        $rev_type    = 'both' unless exists $rev_types{$rev_type};
        return @{$rev_types{$rev_type}->()};
    }
[download]

Though as noted above, the $rev_types should probably be moved outside of the sub.

If necessary, you can also write some code for generating a hash key based upon the difficult logic. This has the advantage of cleanly encapsulating the logic for each case rather than embedding it in the if/elsif/else constructs.

Cheers,
Ovid

New address of my CGI Course.

I guess its time to make my standard plug to use Exporter::Dispatch... although this case is probably too simple to warrant the module.

I believe you are correct, though mine is a rather simplistic example. Data driven programming is a powerful, under-utilized tool. State machines are a closely-related example of this if you'd like to read more about them.

Cheers,
Ovid

New address of my CGI Course.

Point here is if has VALUE ... dispatch tables are an awesome trick, and I'm glad you have discovered them, but sometimes the blocks of code are incredibly unlike, or the tests don't (yes really) map down to scalars.

What you should have said was "sometimes dispatch tables are better than if", not "friends don't let friends use if". That, my friend, is the "if something is good, more of something must be better" fallacy. Much like what happened to the XP crowd... Dispatch tables and FP are good. Nested ifs can usually be a sign problem, but mapping ifs to hashes just makes hashes of virtual ifs...

Anyhow, closures rock. Use them. In many cases, not using an if is a gateway to a higher plane of existance (as is passing functions around), but don't be afraid of if.

Ut oh, someone has decided to claim "if considered harmful".

If you're referring to my node "if" Considered Harmful in OO programming, I made it clear that this was a specific case of "if" statements being used inappropriately in a particular context. It was not a blanket condemnation of the "if" statement. I made that very clear in the node in question. I chose the hackneyed "considered harmful" title because I have a sense of humor, not a sense of dogmatism.

mapping ifs to hashes just makes hashes of virtual ifs...

I'm sorry, but I disagree. By using a dispatch table, one can frequently add another case to the table and it just works. It's frequently faster. It's a clearer specification of behavior. As your if/elsif/else construct grows, the dispatch solution becomes easier to read and maintain. As my first example shows, it's easy to get incorrect default behavior with if chains and sometimes you have to read through the chain to figure out where your particular statement belongs.

I would not argue that we shouldn't use "if" (my "friends don't let friends ..." comment was meant tongue in cheek.) Instead, I argue that we should take a close look at the logic involved with "if" statements and find ways to eliminate such logic, if feasible. It's far better to be able to create a behavior specification rather than potentially convoluted logic which hopefully covers all possibilities.

Cheers,
Ovid

New address of my CGI Course.

Yep, I agree with that, and I was referring to it. "Considered harmful" should be "considered harmful". Tongue and cheek rarely works on the internet, hence confusion.

As your if/elsif/else construct grows, the dispatch solution becomes easier to read and maintain

No doubt about it.

But what about

if ($self->hungry()) {
  # go eat
}
elsif ($theater->open() && $self->wants_movie()) {
  # hunger is more important than movie
}
elsif ($cable->has_good_tv_show()) {
  # watch programming shows only, of course
}
[download]

Bad code, agreed ... but that doesn't translate into a dispatch table. I am quite the fan of dispatch tables.

Dispatch tables work great for scalars if you are only testing scalars, usually your conditionals are more complex.

I too, use the ternary operator most of the time (but never nested), as many things more complex imply too complex ... hence they can be redesigned ... but not always.

Sometimes efficiency is important, and sometimes an if needs to be. Heck, sometimes if you pass through once a switch is what you want (but hey, perl doesn't have a good switch that is implemented without a source filter).