I seem to see this often enough in my code and others. There’s nothing about it that seems horribly wrong, but it annoys me as it looks like it can be done better. I suppose a case statement, might make a little more sense, but often variable is a type that does not work well or at all with case statements (depending on language)
If variable == A
if (Flag == true)
doFooA()
else
doFooA2
else if variable == B
if (Flag == true)
doFooB()
else
doFooB2
else if variable == C
if (Flag == true)
doFooC()
else
doFooC2
It seems there’s multiple ways to “factor” this, such as 2 sets of if-elses, where one set handles when Flag == true.
Is there a “good way” to factor this, or perhaps when this if-else algorithm happens it usually means you are doing something wrong?
5
It could be handled with polymorphism.
factory(var, flag).doFoo();
Whenever you have a bunch of if/else checks on the type of something, you might consider centralizing the if/else check in a factory method, then calling doFoo() polymorphically. But this could be over-kill for a 1-off solution.
Maybe you could create a key/value map where the key is var/flag, and the value is the function itself.
do[var, flag]();
2
Multiple nested ifs increase the cyclomatic complexity of the code. Up to recently, having multiple exit points in a functions was cosidered bad structured code, but now, as long as the code is simple, and short, you can do so, making the code trivial to read:
if (variable == A && Flag) {
doFooA();
return;
}
if (variable == A) {
doFooA2();
return;
}
if (variable == B && Flag){
doFooB();
return;
}
if (variable == B){
doFooB2();
return;
}
if (variable == C && Flag){
doFooC();
return;
}
if (variable == C){
doFooC2();
}
return;
another option is to combine if and switch. This is not superior to your nested if technique, but can reduce the number of duplicate tests (if the switch optimizes to a jump table).
if (flag)
{
switch (variable)
{
case A:
... blah
break;
case B:
... blah
break;
case C:
... blah
break;
default:
... log an error.
... maybe do a default action.
break;
}
}
else // flag == false
{
switch (variable)
{
case A:
... blah
break;
case B:
... blah
break;
case C:
... blah
break;
default:
... log an error.
... maybe do a default action.
break;
}
Well, there’s always this …
if variable == A && Flag
doFooA()
else if variable == A
doFooA2
else if variable == B && Flag
doFooB()
else if variable == B
doFooB2
else if variable == C && Flag
doFooC()
else if variable == C
doFooC2
But frankly, I think the original code isn’t half bad in the first place.
Use polymorphism and a rule array
interface IRule() {
boolean applicable(args...);
obj apply(args...);
}
static final Array<IRule> rules = [new MeaningfulNameRule1(), new MeaningfulNameRule2(), ...];
/* where */
class MeaningfulNameRuleX extends IRule{ /* */ }
/* In your method */
for (rule in rules) {
if (rule.applicable(a,b,c)){
return rule.apply(e,f,g);
}
}
Or as mike30 suggested: If the rule conditions can easily form a key then a hashmap is the best way to go.