Replacing an "if" statement with inheritance is Kool-Aid smell. I've seen it done. I've seen the really bad consequences several times (code that is much harder to understand or to even find the right code to read and tying things in knots so fixing a minor problem requires serious restructuring). So don't interpret that as the lesson.
But I'm not sure why people express the good lesson as "replace 'if'". The good lesson is to keep the logic about an object inside the class. Having logic outside the class means that you have to repeat that logic each time you use the class and that it is harder to fix the logic or change the logic. It makes the code that uses the class more complex and it makes the class harder to understand because you have to go read places that use it in order to see some of the logic. And it requires people get this logic right each time they use the class, which is a waste of energy and encourages bugs.
The bad design smell for me is when you can detect non-trivial knowledge of the class' structure in code outside of the class.
For example, we have blacklists and whitelists. A good design would be to have the request-handling code look like:
if( is_blacklisted($source,$dest,$acct,$parent,$optout) ) {
But our code currently has a big block of logic inside the method for handling requests that demonstrates a ton of knowledge about how the blacklists and whitelists are organized (there are global lists and per-account lists, etc.). That block of code calls two other methods in the request-handling class: one checks against a passed-in list of whitelists, the other checks against a passed-in list of blacklists.
There are several problems with that.
- There is a bloat of code inside the already-too-complex request-handling method. That should just be a single method call.
- There are two methods that are specific to blacklists/whitelists inside the method-handling class. Those should be in a different class.
- The code for setting up these lists is not in the same class as the code for consuming these lists.
- This structure doesn't actually include an interface to the lists so you can't easily tell which inputs determine if a request gets blacklisted (the source, the destination, the destination's account, the parent of that account, and whether the destination opted out of the global blacklist).
- The is_whitelisted and is_blacklisted methods share nearly identical structures.
Note that these problems also mean it is impossible to write unit tests for the list-consuming code.
So, a much better design would have created a separate class for dealing with these lists and the classes that use this new class would do so by calling a single method in isolation for each such use.
Also, the is_blacklisted and is_whitelisted methods should be combined because they share way too much structure:
sub is_listed {
my( $self, $source, $color, @lists ) = @_;
croak ...
if $color !~ /^(black|white)\z/;
$color .= "list";
for my $list ( @lists ) {
next # Let caller may pass in $parent even if empty
if ! $list;
if( $self->lookup($color,$list,$source) ) {
$self->verbose("Found $source on $list $color");
if( $color eq "blacklist" ) {
$self->increment_reject_count($source);
}
}
}
}
The only difference between the two methods was that the is_blacklisted method, when it finds a match, it increments a count and sets a flag on the request. The setting of the flag should be moved to the caller:
my $is_blacklisted = App::Blacklist->is_blacklisted(
$source, $dest, $acct, $parent, $optout,
);
if( $is_blacklisted ) {
# Next line used to be in $self->is_blacklisted():
$self->set_was_blacklisted();
$self->reject();
return;
}
But this also serves as an example where it is an improvement to replace two methods with an 'if'. The 'if' is using knowledge of the structure of the class inside the implementation of the class so it isn't a violation of the good idea behind the badly worded "'if's are bad" meme.
I also found it amusing that chromatic's best example in explaining "Replace Conditional with Polymorphism" was that:
$item->save();
is better than:
$item->save() if $item->is_dirty();
Because if he had replaced that 'if' with polymorphism, then that would mean that he has two different classes, a "not dirty" class where save() is a no-op and a "dirty" class where save() does the saving and then causes the object to be transformed into an object of a different class, the "not dirty" class.
And that is a great example of how replacing an 'if' with polymorphism can be just a terrible thing to try to do.
The article chromatic linked to is actually talking about something much more specific than what chromatic actually discussed. For that very specific case of "if foo isa bar then foo.barkLikeABar() else foo.barkLikeABiff()", you aren't really replacing the 'if' with polymorphism. You are just fixing the polymorphism so you can remove the bad duplication (and also the 'if').
So, if you call multiple methods on a single object in one block of code outside of that object's class, then you should consider whether there is a good description for what those multiple calls are doing with that object and whether you should just have a method that does that. It can lead to more cohesive, modular class design.
A bad class is like a musket, a good class is like a modern pistol:
$musket->insert( $powder );
$musket->insert( $ball );
$musket->tamp();
$musket->replace_cap( $cap )
if $musket->get_cap()->is_expended();
$musket->aim( $target );
$musket->pull_trigger();
# vs
$pistol->shoot( $target );
$pistol->shoot( $target );
| 