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; # 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'"; } }
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; 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'"; }
And so we don't have to recreate the closures every time the function is called, we make the function itself a closure too.# 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'" +}(); }
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 }, ); }
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.
In reply to Re: Rethinking Your Program's If-Thens
by Aristotle
in thread Rethinking Your Program's If-Thens
by princepawn
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |