G'day:
There was a section in last week's Working Code Podcast: Book Club #1 Clean Code by "Uncle Bob" Martin (pt2) where the team were discussing switch statements being a code smell to avoid in OOP (this is at about the 28min mark; I can't find an audio stream of it that I can deep-link to though). I didn't think they quite nailed their understanding of it (sorry team, I don't mean that to sound patronising), so afterwards I asked Namesake if it might be useful if I wrote an article on switch as a code smell. He confirmed that it might've been more a case of mis-articulation than not getting it, but I ought to go ahead anyhow. So I decided to give it some thought.
Coincidentally, I happened to be looking at some of Adam's own code in his Semaphore project, and something I was looking at the test for was… a switch statement. So I decided to think about that.
I stress I said I'd think about it because I'm def on the learning curve with all this stuff, and whilst I've seen some really smell switch statements, and they're obvious, I can't say that I can reason through a good solution to every switch I see. This is an exercise in learning and thinking for me.
Here's the method with the switch in it:
private boolean function ruleMathIsTrue(required any userAttributeValue, required string operator, required any ruleValue){
switch (arguments.operator){
case '=':
case '==':
return arguments.userAttributeValue == arguments.ruleValue;
case '!=':
return arguments.userAttributeValue != arguments.ruleValue;
case '<':
return arguments.userAttributeValue < arguments.ruleValue;
case '<=':
return arguments.userAttributeValue <= arguments.ruleValue;
case '>':
return arguments.userAttributeValue > arguments.ruleValue;
case '>=':
return arguments.userAttributeValue >= arguments.ruleValue;
case 'in':
return arrayFindNoCase(arguments.ruleValue, arguments.userAttributeValue) != 0;
default:
return false;
}
}
First up: this is not an egregious case at all. It's isolated in a private method rather than being dumped in the middle of some other logic, and that's excellent. The method is close enough to passing a check of the single-responsibility principle to me: it does combine both "which approach to take" with "and actually doing it", but it's a single - simple - expression each time, so that's cool.
What sticks out to me though is the repetition between the cases and the implementation:
They're mostly the same except the three edge-cases:
- = needs to map to ==;
- in, which needs a completely different sort of operation.
- Instead of just throwing an exception if an unsupported operator is used, it just goes "aah… let's just be false" (and return false and throwing an exception are both equally edge-cases anyhow).
This makes me itchy.
One thing I will say for Adam's code, and that helps me in this refactoring exercise, is that he's got good testing of this method, so I am safe to refactor stuff, and when the tests pass I know I'm all good.
My first attempt at refactoring this takes the approach that a switch can often be re-implemented as a map: each case is a key; and the payload of the case is just some handler. This kinda makes the method into a factory method (kinda):
operationMap = {
'=' : () => userAttributeValue == ruleValue,
'==' : () => userAttributeValue == ruleValue,
'!=' : () => userAttributeValue != ruleValue,
'<' : () => userAttributeValue < ruleValue,
'<=' : () => userAttributeValue <= ruleValue,
'>' : () => userAttributeValue > ruleValue,
'>=' : () => userAttributeValue >= ruleValue,
'in' : () => ruleValue.findNoCase(userAttributeValue) != 0
};
return operationMap.keyExists(operator) ? operationMap[operator]() : false
OK so I have a map - lovely - but it's still got the duplication in it, and it might be slightly clever, but it's not really as clear as the switch.
Next I try to get rid of the duplication by dealing with each actual case in a specific way:
operator = operator == "=" ? "==" : operator;
supportedComparisonOperators = ["==","!=","<","<=",">",">="];
if (supportedComparisonOperators.find(operator)) {
return evaluate("arguments.userAttributeValue #operator# arguments.ruleValue");
}
if (operator == "in") {
return arrayFindNoCase(arguments.ruleValue, arguments.userAttributeValue);
}
return false;
This works, and gets rid of the duplication, but it's way less clear than the switch. And I was laughing at myself by the time I wrote this:
operator = operator == "=" ? "==" : operator
I realised I could get rid of most of the duplication even in the switch statement:
switch (arguments.operator){
case "=":
operator = "=="
case '==':
case '!=':
case '<':
case '<=':
case '>':
case '>=':
return evaluate("arguments.userAttributeValue #operator# arguments.ruleValue");
case 'in':
return arrayFindNoCase(arguments.ruleValue, arguments.userAttributeValue) != 0;
default:
return false;
}
Plus I give myself bonus points for using evaluate in a non-rubbish situation. It's still a switch though, innit?
The last option I tried was a more actual polymorphic approach, but because I'm being lazy and CBA refactoring Adam's code to inject dependencies, and separate-out the factory from the implementations, it's not as nicely "single responsibility principle" as I'd like. Adam's method becomes this:
private boolean function ruleMathIsTrue(required any userAttributeValue, required string operator, required any ruleValue){
return new BinaryOperatorComparisonEvaluator().evaluate(userAttributeValue, operator, ruleValue)
}
I've taken the responsibility for how to deal with the operators out of the FlagService class, and put it into its own class. All Adam's class needs to do now is to inject something that implements the equivalent of this BinaryOperatorComparisonEvaluator.evaluate interface, and stop caring about how to deal with it. Just ask it to deal with it.
The implementation of BinaryOperatorComparisonEvaluator is a hybrid of what we had earlier:
component {
handlerMap = {
'=' : (operand1, operand2) => compareUsingOperator(operand1, operand2, "=="),
'==' : compareUsingOperator,
'!=' : compareUsingOperator,
'<' : compareUsingOperator,
'<=' : compareUsingOperator,
'>' : compareUsingOperator,
'>=' : compareUsingOperator,
'in' : inArray
}
function evaluate(operand1, operator, operand2) {
return handlerMap.keyExists(operator) ? handlerMap[operator](operand1, operand2, operator) : false
}
private function compareUsingOperator(operand1, operand2, operator) {
return evaluate("operand1 #operator# operand2")
}
private function inArray(operand1, operand2) {
return operand2.findNoCase(operand1) > 0
}
}
In a true polymorphic handling of this, instead of just mapping methods, the factory method / map would just give FlagService the correct object it needs to deal with the operator. But for the purposes of this exercise (and expedience), I'm hiding that away in the implementation of BinaryOperatorComparisonEvaluator itself. Just imagine compareUsingOperator and inArray are instances of specific classes, and you'll get the polymorphic idea. Even having the switch in here would be fine, because a factory method is one of the places where I think a switch is kinda legit.
One thing I do like about this handling is the "partial application" approach I'm taking to solve the = edge-case.
But do you know what? It's still not as clear as Adam's original switch. What I have enjoyed about this exercise is trying various different approaches to removing the smell, and all the things I tried had smells of their own, or - in the case of the last one - perhaps less smell, but the code just isn't as clear.
I'm hoping someone reading this goes "ah now, all you need to do is [this]" and comes up with a slicker solution.
I'm still going to look out for a different example of switch as a code smell. One of those situations where the switch is embedded in the middle of a block of code that then goes on to use the differing data each case prepares, and the code in each case being non-trivial. The extraction of those cases into separate methods in separate classes that all fulfil a relevant interface will make it clearer when to treat a switch as a smell, and solve it using polymorphism.
I think what we take from this is the knowledge that one ought not be too dogmatic about stamping out "smells" just cos some book says to. Definitely try the exercise (and definitely use TDD to write the first pass of your code so you can safely experiment with refactoring!), but if the end result ticks boxes for being "more pure", but it's at the same time less clear: know when to back out, and just run with the original. Minimum you'll be a better programmer for having taken yerself through the exercise.
Thanks to the Working Code Podcast crew for inspiring me to look at this, and particularly to Adam for letting me use his code as a discussion point.
Righto.
--
Adam
