Perl 101: avoid "elsif"
We had some code which looked (sort of) like this:
if ( $ondemand->service eq 'iplayer_broadband_streaming' ) {
$self->is_available_vp6_stream(1);
}
elsif ( $ondemand->service eq 'iplayer_streaming_h264_flv' ) {
$self->is_available_h264_stream(1);
}
elsif ( $ondemand->service eq 'iplayer_streaming_h264_flv_high' ) {
$self->is_available_h264_high_stream(1);
}
elsif ( $ondemand->service eq 'iplayer_streaming_h264_flv_hd' ) {
$self->is_available_h264_low_stream(1);
}
elsif ( $ondemand->service eq 'iplayer_streaming_h264_flv_lo' ) {
$self->is_available_h264_hd_stream(1);
}
elsif ( $ondemand->service eq 'iplayer_mobile_wmv' ) {
$self->is_available_iplayer_mobile_wmv(1);
}
elsif ( $ondemand->service eq 'iplayer_broadband_download' ) {
$self->is_available_http_wmv(1);
}
else ( $ondemand->service eq 'iplayer_download_http_h264_air' ) {
$self->is_available_elektra_sd(1);
}
As a general rule of thumb, I recommend that when you use an "if" statement, you keep it as simple as possible:
if true
do something
end if
Or:
if true
do something
else
do something else
end if
Long if/elsif/end chains like you see above have a few issues:
- Duplicate code
- Harder to maintain
- Confusing to read (there's a probable bug in that code)
Let's look at these.
Obviously we have a lot of duplicate code here. For example, what if, instead of $ondemand->service, we have to change that to $ondemand->version->service? It gets annoying to change all of those, so factor this out first:
my $service = $ondemand->service;
if ( $service eq 'iplayer_broadband_streaming' ) {
$self->is_available_vp6_stream(1);
}
elsif ( $service eq 'iplayer_streaming_h264_flv' ) {
$self->is_available_h264_stream(1);
}
elsif ( $service eq ... ) {
# and so on
}
Already this is a little bit cleaner, but what happens when we have to log which methods we're setting to true?
my $service = $ondemand->service;
my $id = $ondemand->id;
if ( $service eq 'iplayer_broadband_streaming' ) {
$self->is_available_vp6_stream(1);
log("Set is_available_vp6_stream to true for ondemand $id");
}
elsif ( $service eq 'iplayer_streaming_h264_flv' ) {
$self->is_available_h264_stream(1);
log("Set is_available_h264_stream to true for ondemand $id");
}
elsif ( $service eq ... ) {
# and so on
}
OK, that's going to get even more tedious. The trick, with simple comparisons in long elsif chains like this, is to use a simple hash.
my %method_for = (
iplayer_broadband_streaming => 'is_available_vp6_stream',
iplayer_streaming_h264_flv => 'is_available_h264_stream',
iplayer_streaming_h264_flv_high => 'is_available_h264_high_stream',
iplayer_streaming_h264_flv_hd => 'is_available_h264_low_stream',
iplayer_streaming_h264_flv_lo => 'is_available_h264_hd_stream',
iplayer_mobile_wmv => 'is_available_iplayer_mobile_wmv',
iplayer_broadband_download => 'is_available_http_wmv',
iplayer_download_http_h264_air => 'is_available_elektra_sd',
);
# Don't need to factor out $service because it's no longer duplicated
if ( my $availability = $method_for{ $ondemand->service } ) {
$self->$availability(1);
log( "Set $availability to true for ondemand " . $ondemand->id );
}
Now we see a couple of things. First, we haven't duplicated out logic, so extending is now trivial. Second, it's easier to see what's actually going on because we've cleaned up our code. Third, if you read through the hash, you might have an easier time of spotting a couple of service/availability methods which don't appear to make sense. It's still not easy (particularly if you don't know the data), but it's easier than with the elsif chain.
When I made a very similar change to our code base, I found myself hesitating for a moment. After all, it's not good to embed data in our code. We want that in config files or the database. But where should that data go? And was the above code a proper distribution of responsibility? And, and ...
Then I kicked myself. The code is cleaner and easier to read and I had forgotten one of the most important rules of refactoring: even if it's not perfect, don't be afraid to make code better.
Freelance Perl/Testing/Agile consultant and trainer. See http://www.allaroundtheworld.fr/ for our services. If you have a problem with Perl, we will solve it for you.
And don't forget to buy my book! http://www.amazon.com/Beginning-Perl-Curtis-Poe/dp/1118013840/
I often see the same pattern arise in my own code. First, it's an if/elsif chain with 2 or 3 options. Then, the options accrue to the point where a hash or array makes more sense. Eventually, I get tired of modifying the code and pushing a new commit every time I need to add support for a new {blurg}, so I migrate the data into the database.
Part of me always thinks I should look for opportunities to skip to the last step right away, but it's hard to know which situations warrant this treatment until after the fact. Sometimes those original 2 branches are all I ever need.
I have a couple of chapters in Mastering Perl that go through this sort of thing pretty thoroughly, showing lots of great dispatch-tably sorts of ways around the long chain of if branches.
Why strings in %method_for instead of subroutine references?
Of course the classic answer to this kind of code pattern is to use polymorphism - but yeah - I agree sometimes this would mean just too many classes, a simple hash is more compact.
I'm disappointed that "avoid" would be the advice. "Take note of," sure. "Beware," OK. But "avoid?" No.
tokumei: there are a couple of reasons, but mainly because the simpler the example, the easier it is to show the point.
tokumei: There are strings rather than code references in dispatch table because dispatch goes through invoking methods on object (those strings are method names), and because it gives us method name for logging (and not only $ondemand->service().
I like your method using a hash. But just to improve the readability a switch statement should do it (when using 5.10 or later).
I have to agree with Chip on this one.
Also, this is example is bad enough. But the real problem with
elsifchains is when the conditions aren’t parallel (and in a long chain ofelsifs, you have to read very very carefully to know they’re not, even when it looks like they should be). Then it requires a truth table to figure out exactly when the later clauses are taken.For fun I'll occasionally take dispatch tables and generate chained if-else code.
The generated code looks gruesome but runs quickly. Of course if you install the code on-the-fly you don't even have to look at it.