in reply to Rethinking Your Program's If-Thens

First reorganize the sub such that all branches are as similar as possible. 
# return a list consisting of correct division and service
# as a function of $dcr_type and @row
sub division_service {
    my $dcr_type = uc shift;
    my $row = shift; # you were copying an array here. not necessary

    if    ($dcr_type eq 'CONTACT') {
        return ('HQ' , undef);
    }
    elsif ($dcr_type eq 'FAQ') {
        return (undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4])
+;
    }
    elsif ($dcr_type eq 'LITERATURE') {
        return (undef, $row->[4]);
    elsif ($dcr_type eq 'ORGANIZATION') {
        return (($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), unde
+f);
    }
    else {
        croak "Unknown \$dcr_type '$dcr_type'";
    }
}

[download]
Our if/elsif condition is now perfectly regular, and we're repeating the return everywhere. We can do better: 
# return a list consisting of correct division and service
# as a function of $dcr_type and @row
sub division_service {
    my $dcr_type = uc shift;
    my $row = shift;

    return
        $dcr_type eq 'CONTACT' ?
            ('HQ' , undef) :
        $dcr_type eq 'FAQ' ?
            (undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4]) :
        $dcr_type eq 'LITERATURE' ?
            (undef, $row->[4]) :
        $dcr_type eq 'ORGANIZATION' ?
            (($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), undef) 
+:
        croak "Unknown \$dcr_type '$dcr_type'";
}

[download]
Now it's a breeze to convert that to a hash of closures: 
# return a list consisting of correct division and service
# as a function of $dcr_type and @row
sub division_service {
    my $dcr_type = uc shift;
    my $row = shift;

    my %map = (
        CONTACT =>
            sub { 'HQ' , undef },
        LITERATURE =>
            sub { undef, $row->[4] },
        FAQ =>
            sub { undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4
+] },
        ORGANIZATION =>
            sub { ($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), un
+def },
    );

    return &{$map{$dcr_type} || croak "Unknown \$dcr_type '$dcr_type'"
+}();
}

[download]
And so we don't have to recreate the closures every time the function is called, we make the function itself a closure too. 
BEGIN {
    my ($row, %map);

# return a list consisting of correct division and service
# as a function of $dcr_type and @row
    sub division_service {
        my $dcr_type = uc shift;
        $row = shift;
        return &{$map{$dcr_type} || croak "Unknown \$dcr_type '$dcr_ty
+pe'"}();
    }

    %map = (
        CONTACT =>
            sub { 'HQ' , undef },
        LITERATURE =>
            sub { undef, $row->[4] },
        FAQ =>
            sub { undef, $row->[2] eq 'Service' ? $row->[3] : $row->[4
+] },
        ORGANIZATION =>
            sub { ($row->[0] eq 'Headquarters' ? 'HQ' : $row->[2]), un
+def },
    );
}

[download]

That's what I'd do at first glance. I haven't got a few decades of experience yet, so if any of those who do can point out some maintainability flaw in this reasoning I'd be glad to listen.

Oh, and I'd name the sub something like get_division_service_for or something similar, so that it's clearer what's happening when someone reads the call.

Update: Added BEGIN to the anonmyous block in the last sample as per Anonymonk's remark.

Makeshifts last the longest.

Replies are listed 'Best First'.
Re: Re: Rethinking Your Program's If-Thens
by Anonymous Monk on Oct 12, 2002 at 17:31 UTC
    Maintainability flaw: using an anonymous array rather than an anonymous hash or named constants. Can you tell me what 3 and 4 mean in your code?

    Also a gotcha, this function will work in a module and may not when buried in the body of your program because %map isn't yet populated. Solutions: put a BEGIN in front of the start of your block, be aware of the limitation, or put a check in division_service that populates %map if it isn't populated yet.

    And style points. I don't like &{...}() as much as (...)->(). Also I don't know what I think of your cute trick with putting the error check inside the dereference rather than doing a traditional exists test.

[reply]

      As far as the array is concerned, since I don't know the meaning of its fields I had to work with what princepawn gave us.

      Thanks for pointing out the module vs main program issue which I omitted even though I was aware of it. I'd prefer the BEGIN in that case: no checks at runtime (CPU efficiency), no extra code to read and write (programmer efficiency).

      You are right about &{$ref}() and I usually prefer $ref->() as well. In this case it was a conscious choice because I felt it read better this way considering I embedded the croak in the lookup.

      I generally dislike exists tests because they usually force me to spell out the same hash lookup twice. At least in this case we can get away with a simple or-test, because a hash value will either not exist or be a coderef, which by definition is true. Now the choice is to either embed the error check in the call like I did (ugly), or use a temporary variable like so - also ugly:

      my $lookup_div_serv = $map{$dcr_type} or croak "...";
return $lookup_div_serv->();

      [download]
      I'd be happy with this if I needed the $lookup_div_serv in muliple places, but I don't. This is basically a case of deciding which ugliness to consider the lesser of evils.

      Makeshifts last the longest.

[reply]
[d/l]
        One point. The BEGIN solution isn't best if your block refers to other functions that might be defined later in your code. Being lazy avoids this issue and generally will Do The Right Thing with less attention on your part. Admittedly with a small run-time penalty.
[reply]

In Section Meditations