Beefy Boxes and Bandwidth Generously Provided by pair Networks
Syntactic Confectionery Delight

A short, "iffy" rant

by Ovid (Cardinal)
on Oct 11, 2004 at 20:19 UTC ( #398284=perlmeditation: print w/replies, xml ) Need Help??

(This has been cross-posted to my use.perl journal.)

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_multip +le/ ], ); 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 (that's the unless in the above code.) 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. This actually happens quite frequently if we look at our "if" statements and figure out how to excise them.

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


New address of my CGI Course.

Replies are listed 'Best First'.
Re: A short, "iffy" rant
by FoxtrotUniform (Prior) on Oct 11, 2004 at 20:28 UTC

    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.

    Yours in pedantry,
    F o x t r o t U n i f o r m

Re: A short, "iffy" rant
by Zaxo (Archbishop) on Oct 11, 2004 at 21:09 UTC

    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')); }
    That way the selection logic and accessor are all bundled up in their own scope where nobody can harm them.

    After Compline,

Re: A short, "iffy" rant
by iburrell (Chaplain) on Oct 11, 2004 at 21:34 UTC
    Using a hash for dispatch table is great when the select is single valued like that. Sort of like a case statement.

    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}->()}; }

      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.


      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.

Re: A short, "iffy" rant
by zby (Vicar) on Oct 12, 2004 at 10:39 UTC
    Isn't it what they call Data Driven Programming?
Re: A short, "iffy" rant
by SpanishInquisition (Pilgrim) on Oct 12, 2004 at 16:45 UTC
    Ut oh, someone has decided to claim "if considered harmful". Be afraid, be very afraid. I once had a room mate (many years ago) whose professor had told them NEVER to use an if, only the trinary, and to make all functions 3 lines long. If he's trying to teach functional programming, I can kind of see the point .. but this was a (dare I say the name) Java (there I said it) class.

    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.


      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 }

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

          A reply falls below the community's threshold of quality. You may see it by logging in.

Log In?

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlmeditation [id://398284]
Approved by FoxtrotUniform
Front-paged by diotalevi
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others rifling through the Monastery: (1)
As of 2023-09-24 22:47 GMT
Find Nodes?
    Voting Booth?

    No recent polls found