Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
6  
could it be possible to pass the Flag variable to the doFooX that could deal with the flag itself? –  Jean-François Côté Jun 10 '13 at 19:11
5  
IMHO, it will still be clearer to read. Then it depend what doFooA and doFooA2 do... –  Jean-François Côté Jun 10 '13 at 19:23
1  
+1 for "clearer to read" –  TruthOf42 Jun 10 '13 at 19:29
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)? –  Keith Thompson Jun 17 '13 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. –  Patrick Hughes Jun 17 '13 at 20:45
show 1 more comment

5 Answers

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]();
share|improve this answer
2  
+1: Table lookups beat nested if-statements almost every time. –  kevin cline Jun 11 '13 at 15:29
1  
I want to say this is the best solution. –  The Muffin Man Jun 11 '13 at 17:52
add comment

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;
share|improve this answer
add comment

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;
}
share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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