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.
if(condition1)
{
   Statement1A;
   Statement1B;
}
else if(condition2)
{
  Statement2;
}
else if(condition3)
{
  Statement3;
}
else 
{
   Statement1A;
   Statement1B;
}

   return;

I would like to refactor that code so that I do not duplicate Statements. I always need to check condition1 before any other condition. (So I cannot just change the order of the conditions). I also do not want to write &&!condition1 after every other condition.

I solved it like this

if(condition1)
{
}
else if(condition2)
{
  Statement2;
  return;
}
else if(condition3)
{
  Statement3;
  return;
}

Statement1A;
Statement1B;
return;

However I do not think an empty if condition will be easily understandable by others (even by me after a while).

What is a better approach?

share|improve this question

marked as duplicate by GlenH7, ChrisF Dec 16 '13 at 23:29

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

5  
Why do you need to check condition1 before any other condition? –  Mike Dec 10 '13 at 13:29
7  
@Mike presumably checking condition1 also causes side effects (yuck) –  jk. Dec 10 '13 at 13:34
2  
@M.C. Refactoring conditional logic is usually something that will be different on a case-by-case basis. In this particular case, you can remove if (condition1) entirely, because it runs the exact same content as the else block. Assuming it doesn't cause side effects as jk suggests. That would be pretty 'yuck' indeed. –  KChaloux Dec 10 '13 at 13:34
2  
@KChaloux this is exactly what I can not do. Executing Statement2 while condition1 applies is not correct in my context –  M.C. Dec 10 '13 at 13:36
2  
@Kieveli checking a condition does not change anything by itself here - which I agree is bad practice. The code says if (A) else if (B). Removing if (A) affects the code flow not because checking A had some side effect that's missing now. You removed the else as well. You changed the code semantically. –  Konrad Morawski Dec 10 '13 at 15:34
show 10 more comments

10 Answers

up vote 27 down vote accepted
+100
notCondition2And3 = !condition2 & !condition3; 
// in place of notCondition2And3 should be some meaningful name
// representing what it actually MEANS that neither condition2 nor condition3 were true

And now:

if (condition1 || notCondition2And3)
{
   Statement1A;
   Statement1B;
   return;
}
if (condition2)
{
   Statement2;
   return;
}
if (condition3)
{
   Statement3;
   return;
}

As I wrote in my comment to Kieveli's answer, I see nothing wrong about multiple returns in a method, if there is no memory management considerations (as it might be the case in C or C++ where you have to release all resources manually before you leave).

Or another approach still. Here's the decision matrix so that we don't mess it up:

F F F - 1
---------
F F T - 3
---------    
F T F - 2
F T T - 2
---------    
T F F - 1
T F T - 1
T T F - 1
T T T - 1

Ts and Fs represent the values of condition1, condition2 and condition3 (respectively). The numbers represent the outcomes.

It makes it clear that it's also possible to write the code as:

if (!condition1 && condition2) // outcome 2 only possible for FTF or FTT, condition3 irrelevant
{
   Statement2;
   return;
}
if (!condition1 && !condition2 && condition3)  // outcome 3 only when FFT
{
   Statement3;
   return;
}
// and for the remaining 5 combinations...
Statement1A;
Statement1B;

Now if we extracted !condition1 (which is present in both ifs), we would get:

if (!condition1)
{
    if (condition2) // outcome 2 only possible for FTF or FTT, condition3 irrelevant
    {
       Statement2;
       return;
    }
    if (condition3)  // outcome 3 only when FFT
    {
       Statement3;
       return;
    }
}
// and for the remaining 5 combinations...
Statement1A;
Statement1B;

Which is almost exactly what Kieveli suggested, only his disdain for early returns caused his implementation to be buggy (as he noted himself), because it wouldn't do a thing if all 3 conditions were false.

Or, we could revert it like so (this probably wouldn't work in every language - it works in C#, for one, since C# allows for equality comparison between multiple variables), now we're virtually back to the first one:

// note that "condition1" on the right side of || is actually redundant and can be removed, 
// because if the expression on the right side of || gets evaluated at all,
// it means that condition1 must have been false anyway:

if (condition1 || (condition1 == condition2 == condition3 == false)) // takes care of all 4 x T** plus FFF (the one special case). 
{
    Statement1A;
    Statement1B;
    return;
}
// and now it's nice and clean
if (condition2) 
{
    Statement2;
    return; // or "else" if you prefer
}
if (condition3)
{
    Statement3;
    return; // if necessary
}
share|improve this answer
8  
+1 for building the decision matrix. –  Bobson Dec 10 '13 at 15:43
1  
Quite a nice solution. I noticed that nesting makes the code overly complicated (hence the sport of bracket hunting, which I think every programmer has done at least once), and unless early return causes problems, I think this is the way to go. –  Vlad Preda Dec 10 '13 at 15:45
4  
The thing I needed was not to repeat conditions nor statements. And your solution (the one starting with !condition1) is a semantically correct solution that does exactly that. –  M.C. Dec 10 '13 at 17:53
1  
@M.C. glad I could help, thanks for accepting the answer –  Konrad Morawski Dec 10 '13 at 17:54
1  
I usually draw a K-Map, similar to the matrix though. But the K-Map suggets condition2==condition3==false(since adding columns is easier than adding a cell), which you already mentioned that you could do. –  Cruncher Dec 10 '13 at 18:29
add comment

I'm not a fan of your empty if and return statements. It becomes tricky to follow, and it's generally frowned upon to have multiple return statements within the body of your code. Given your goals, I would do something like this:

if ( ! condition1 )
{
   if ( condition2 )
      Statement2;
   else if ( condition3 )
      Statement3;
}
else
{
   Statement1A;
   Statement1B;
}

Note: This solution is wrong! Consider !condition1 && !condition2 && !condition3

share|improve this answer
5  
Personally, I don't mind multiple return statements, but only in the case where you're validating input at the very beginning of a method. –  Kieveli Dec 10 '13 at 13:40
25  
it's generally frowned upon to have multiple return statements within the body of your code - I don't think it's generally frowned upon. It's frowned upon among people used to languages with manual memory management such as C (because multiple points of return make it harder to make sure that all resources are released). Personally I dislike this oldschool habit thoughtlessly transplanted into C# (or Java) code, at the cost of having to maintain a cascade of nested ifs and elses, plus a bunch of local variables, totally unnecessary otherwise –  Konrad Morawski Dec 10 '13 at 14:42
10  
You know.... I'm not sure my answer is correct. What about the case where !condition1 and !(condition2 or condition3). That should have a statement1a and statement1b as well. –  Kieveli Dec 10 '13 at 14:50
2  
Yes, if all 3 are false then no Statement will be executed at all. –  Konrad Morawski Dec 10 '13 at 15:14
1  
I upvoted this before @Kieveli reconsidered things and discovered the answer was wrong. (Shame on me for not looking deeper!) I'm keeping my upvote because "this answer is useful": it shows how slippery even the simple stuff can be if you're not paying attention! Keep your wits about you, people! –  paul Dec 11 '13 at 17:55
show 2 more comments

Repeating statements is only a problem in so much as you're replicating an aspect of your code -- in this case, "do Statement1A, then do Statement1B".

A cleaner re-factoring is to move each of those blocks into separate methods, even if the other method is only two lines.

void _statement1() {
  Statement1A;
  Statement1B;
}

Having a such a method allows you to simply reference it in places where you would have repeated yourself.

if(condition1) 
{
  _statement1()
} 
else if(condition2) 
{
  Statement2;
}
else if(condition3)
{
  Statement3;
}
else 
{
  _statement1()
}

Of course, this raises the follow-up question of "when should I use a method instead of just repeating myself?", which has considerations all of its own. (I prefer "whenever possible", but also flagrantly violate that rule depending on the language.)

share|improve this answer
add comment

I like this way:

        if (!condition1)
        {
            if (condition2)
            {
                Statement2();
                return;
            }
            else if (condition3)
            {
                Statement3();
                return;
            }
        }

        Statement1A();
        Statement1B();
        return;

EDIT:

I like even more this way because I have only 1 return and 2 IF statements :

        if (!condition1 && condition2)
            Statement2();
        else if (!condition1 && condition3)
            Statement3();
        else
        {
            Statement1A();
            Statement1B();
        }
        return;
share|improve this answer
add comment

Maybe it's just me, but I find a condition like if (!a && b) relatively difficult to read, and generally prefer if (b && !a), when possible (i.e., unless I'm depending on short-circuit evaluation so b will only be evaluated if a was false).

Based on that, I'd probably write it as:

if (condition2 && !condition1)
   statement2;
else if (condition3 && !condition1) 
   statement3;
else {
    statement1A;
    statement1B;
}

As I said though, while this is fine if you only care about which statements are evaluated, it doesn't work if you also care about which conditions are evaluated.

share|improve this answer
add comment

There's another way. I would prefer to keep the original control flow. Then, the work that is done twice, can be put it in a function, and then we invoke the function from two different places:

function X()
{  
  if(condition1)
  {
     HandleDefault();
     return;
  }

  if(condition2)
  {
     Statement2;
     return;
  }

  if(condition3)
  {
    Statement3;
    return;
  }

  HandleDefault();
  return;

}
/// 


function HandleDefault(   )
{
    Statement1A;
    Statement1B;
}

Often, if there are blocks of statements that go together, it is possible to give the function a meaningful name, increasing even more the readability of the code. But if you don't have a meaningful name, you can still make a private function with a less descriptive name (as I did in my example).

The reason why I think this is the best solution because it retains the order of conditionals, while it does achieve that you have only one copy of Statement1A and Statement1B to maintain, and it does so without introducing any nesting constructs or temporary variables. This reduces the amount of stuff you need to keep mentally track of when reading this code.

Now, there are languages (most notably C) where early returns can't always be used, but in any other mainstream language I can think of you can.

share|improve this answer
add comment

Two approaches which have not yet been mentioned are to rewrite the statements as expressions (if they are amenable to such) or (dare I say it) using a little word that starts with g. There are cases where I would consider each of those the best alternative.

In the case of expressions, the code would be something like:

if (condition1 ||
     condition2 ? (action2(),1) :
     condition3 ? (action3(),1) :
     1)
  action1;

Rather nasty, and not generally something I would use, but it may be reasonable in cases where condition2 and action2 are both encapsulated in a method that attempts an operation and indicates whether it succeeds and likewise condition2 and action3. In that case, it could become:

if (cantTryAnything || (
     tryFirstThing() != FIRST_TRY_SUCCESS &&
     trySecondThing() != SECOND_TRY_SUCCESS
)
{
  default actions here
}

Do the default actions if either I couldn't try anything, or everything that I tried, failed.

Note that simply copying out the actions or factoring them to a different method would generally be preferable to the above style of logic or a g___. but if code in "default actions here" would need to manipulate local variables, and its exact behavior might change, encapsulating the code into an outside method may be difficult and duplicating the code would pose a risk of the duplicate versions might be changed so as to no longer match perfectly. Further, the notion that a certain control-flow statement is harmful centers around the fact that most real-world control-flow problems can be mapped nicely into common language constructs, and that when a real-world problem maps well to a language construct one should use it. If the particular program flow required by an application requires identical behavior under multiple combinations of conditions, I would posit that it's better to explicitly force execution to a common point in all such conditions than fudge around the issue.

share|improve this answer
add comment

Based on given requirements, I like this version:

if (condition1 || (!condition2 && !condition3)) {
    Statement1A;
    Statement1B;
    return;
}

if (condition2) {
    Statement2;
    return;
}

Statement3;
return;

An alternative without inner returns:

if (condition1 || (!condition2 && !condition3)) {
    Statement1A;
    Statement1B;
} else if (condition2) {
    Statement2;
} else if (condition3) {
    Statement3;
}
return;
share|improve this answer
1  
-1 Reading this means tracking all of the returns. If I had folding on I might assume Statement3 is always run. It wouldn't be so bad if all the returns were inside if blocks, but as is I don't think it is an improvement. –  Lego Stormtroopr Dec 10 '13 at 22:33
add comment

Two things come to mind :

Switch-case

though not always applicable it can make the code easier to read and maintain. Basically capture the different condition in an enum and create a function to analyse the context to produce the correct enum value, then switch it and run the appropriate code.

you will achieve two things :

  • seperate the logic code from the execution. This isolation will make it easier to fix bugs in each parts.
  • easy to read control flow. The switch structure is pretty intuitive, especially so if you carefully name the enum values.

Functions

Capture each block of code in a function and in the beginiing of the function put some code to decide if you should execute or not.

Here you do not get the seperation mentioned above but you do break the IF-ELSE code flow stcructure in more maneagable bits. Plus should a bug occur it will be faster to fix as it will be isolated to a single block of code.

Biggest advantage though is that a change in any one of these functions will not affect other parts of the code.

Functions do have an edge over the switch case is that they could be added dynamically. One natural extention would be something akin to the command pattern where a first part of the code analyse the context and queues actions in a list then a second part simply execute these actions sequentially.

Regardless of chosen strategy KEEP IT SIMPLE sometimes a series of if-else is enough and should just remain as-is. As a rule of thumbs if I return in the same code to fix what seems like the same problem or if I find myself often having to scratch my head to add yet a noew condition then perhaps it`s time to seek alternatives such as ones mentionned above.

share|improve this answer
add comment

Use HashMap with reflection in java.

public class DoktorBilgiKaydet {

    private HashMap<Boolean, String> map = new  HashMap<Boolean, String>(); 

    private DoktorBilgiKaydet()
    {
       createMap();
    }

    public static DoktorBilgiKaydet create()
    {
        return new DoktorBilgiKaydet();
    }


    public void createMap()
    {
        if (map.size() == 2) return;
        map.put(false, "insert");
        map.put(true, "update");  
    }

    public void insert()
    {
        // to do
    }

    public void update()
    {
        // to do 
    }

    public void execute()
    {
        Boolean result = dao.isRecordExist(); 
        String methodName=map.get(result);
        try{
            this.getClass().getMethod(methodName).invoke(this);
        } catch (IllegalArgumentException e) {
            // handle exception
        } catch (SecurityException e) {
            // handle exception
        } catch (IllegalAccessException e) {
            // handle exception
        } catch (InvocationTargetException e) {
            // handle exception
        } catch (NoSuchMethodException e) {
            // handle exception
        }
    }
}

Usage :

DoktorBilgiKaydet.create().execute();
share|improve this answer
    
Programmers is tour conceptual questions answers are expected to explain things. Throwing code dumps instead of explanation is like copying code from IDE to whiteboard: it may look familiar and even sometimes be understandable, but it feels weird... just weird. Whiteboard doesn't have compiler –  gnat Dec 13 '13 at 11:32
2  
I like how this answer doesn't address the problem. Whimsical coding. –  Kieveli Dec 13 '13 at 13:52
add comment

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