I have a few times needed to try and explain to a developer why they should change a class which has multiple instances of
switch(m_type) {
case TYPE_A:
...
break;
case TYPE_A:
...
break
...
}
Where m_type is a (final) reference to an enum defining what type of instance this is.
Whenever I see this I think it's time to split the class out into multiple subclasses where the concrete class you create then defines the type. The argument I always get back is 'It didn't seem worth creating all these classes with hardly any code in, what are they going to have in there? like 2 methods or something!'
The best explanation I've seen is that unnecessary control logic should be avoided. If the code can be restructured to remove or avoid a branch, if, switch, etc. it should. If not, then use the control statement that is the most appropriate. From here but I'm not convinced this will be well received since the dev in question always thinks their code isn't too complex anyway so probably won't get why it matters.
This is a common enough mistake though with a very common attitude to why it exists, has anyone had any luck helping a developer understand the mistake they are making?
Thanks,
Robin
You're asking for justification for the refactoring: Replace Conditional with Polymorphism, or the more specific: Replace Type Code with Subclasses. The Anti-If Campaign has some relevant articles.
The sourcemaking website says:
The advantage of Replace Type Code
with Subclasses is that it moves
knowledge of the variant behavior from
clients of the class to the class
itself. If I add new variants, all I
need to do is add a subclass. Without
polymorphism I have to find all the
conditionals and change those. So this
refactoring is particularly valuable
when variants keep changing.
I don't think there's much to add to that.
The reason is that the programmer has to write the case statements himself, whereas the compiler provides the decision-making when dispatching method calls among subclasses. Less code means less bugs. Especially in a C-like language like Java, where cases fall through. Every case presents an opportunity to (a) put in the wrong case value and (b) forget the break.
Another reason is that when the next programmer comes along and wants to add behavior #2 to this part of the code, that programmer will have to write another switch that duplicates the control logic of the first, then keep them in sync, whereas with the subclass strategy, he can just add a new method.
Of course there are always exceptions. The switch statement is not completely useless. If the code in question really is trivial then maybe let it go. I think that code should be clear and straightforward first, architecturally pure second.
Make sure they write thorough unit tests for the code in question. Then explain that when a new 'type' is added in future and they need to change this code, they will now have broken unit tests that they need to fix.
The Single-Choice principle states that, when a list of possible options exists in a program, only one place in the program should know its exhaustive list. It's an important special case of DRY. Ask whoever keeps using switch statements what happens if you need to add to the list of possibilities. The beauty of polymorphism is that code can be written that will handle any possible behavior without knowing the exhaustive list of behaviors.
Lead by example, show some places where you have removed duplicate switch statements.
Tell him that he needs to use an extra variable to store the object type. Tell him about the vtable (if using C++) and explain how that idiom is baked into the language and thus idiomatic.
Well this is why object oriented programing was invented in the first place. Tell him with static binding, which a case statement is, any new type introduced needs old code to be rewritten, recompiled, retested and redistributed.
Whereas with classes and virtual functions new code can be called by old code.
The code you described is in direct violation of the Open/Closed Principle (the "O" in SOLID). With respect to OOP, that should be enough to make your case. ;-)