10

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
  • 6
    could it be possible to pass the Flag variable to the doFooX that could deal with the flag itself? Jun 10, 2013 at 19:11
  • sure but then you just have a method doFooX, that evaluates if the flag is true and then does either doFooX1 or doFooX2
    – TruthOf42
    Jun 10, 2013 at 19:18
  • 5
    IMHO, it will still be clearer to read. Then it depend what doFooA and doFooA2 do... Jun 10, 2013 at 19:23
  • 2
    Why write if (Flag == true) rather than just If (Flag)? If you think that If (Flag == true) is better, why not if ((Flag == true) == true)? Jun 17, 2013 at 20:13
  • 1
    The most important take-away from most of the answers below is that simplicity and legibility are far more important than clever tricks when it comes to logical flow and maintaining the code into the future. Jun 17, 2013 at 20:45

5 Answers 5

18

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
  • 2
    +1: Table lookups beat nested if-statements almost every time. Jun 11, 2013 at 15:29
  • 1
    I want to say this is the best solution. Jun 11, 2013 at 17:52
6

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;
3

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;
}
0

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.

0

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.