13
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?

gnat
  • 21,213
  • 29
  • 113
  • 291
M.C.
  • 257
  • 5
    Why do you need to check condition1 before any other condition? – Mike Dec 10 '13 at 13:29
  • This is really an implementation detail and I am trying to make the question as general as possible – M.C. Dec 10 '13 at 13:32
  • 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
  • 1
    If checking a condition does indeed change things, then you're in for some unexpected behaviour at some point! No one will be expecting a condition check to do anything other than check conditions - including yourself in 6 months. That's when buggy code will be added. – Kieveli Dec 10 '13 at 13:54
  • 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
  • @Mike One example where we're using that structure in our code is when a business decision dictates a series of fallbacks, but if none of them apply, use the original one with incomplete data. In our particular case, we opted to keep them separate, since that particular business decision had already changed twice during development. – Izkata Dec 10 '13 at 18:55
  • Can someone give me an example when a condition necessarily does change things and can't be refactored? – Sven Dec 10 '13 at 19:06
  • @Sven condition1 may be the return value of a method call that also changes state. Also some other things, like in Java, condition1 might be a volatile variable and checking its value results in other changes being seen. – Radiodef Dec 10 '13 at 19:11
  • I think a good answer to this question depends on what the code is actually doing. For instance, if those statements are changing some data collection, you could default the data collection to a certain state then modify it as you move through the if-else block. If those statements do something else, you may need to redactor their logic to make it clean. This question seems almost too general to be useful- I don't think there exists a single applicable use case. – MirroredFate Dec 10 '13 at 20:55
  • @KonradMorawskiL: I don't understand your comment. If checking conditions has no side effects in this code then removing check for condition1 does not change anything. Code works same but is cleaner. – Piotr Perak Dec 10 '13 at 21:04
  • 2
    @Peri: Not Necessarily. What of the case condition1&&condition2? According to the OP, running statement2 would be the wrong behavior, which is what your version would do. – SailsMan63 Dec 11 '13 at 04:11
  • You are right. My eyes saw if's not else if's. I couldn't find my comment and delete it after I realized that off the computer. – Piotr Perak Dec 11 '13 at 10:00
  • Wouldn't it be easier and clean this way: if(condition2) { Statement2; } else if(condition3) { Statement3; } else { Statement1A; Statement1B; } return; – theinsaneone Dec 11 '13 at 03:36
  • @M.C. In your refactored code, why do you even need to check condition1? As far as I can tell from the original, both if(condition1) and else blocks execute the exact same statements. Hence you only need to check condition2 and condition3, and simply remove condition1 altogether: if (condition2) { Statement2; return; } else if (condition3) { Statement3; return; } Statement1A; Statement1B; return;. There is no need for an empty if(condition1) block in your refactored code (assuming there are no side effects being removed). – ADTC Sep 10 '14 at 05:08
  • @M.C. I noticed, you mentioned there are side effects ("I always need to check condition1 before any other condition."). Sorry, I just noticed. In this case, you can follow Conrad's third approach (nested if blocks which would mean "I only check condition2 and condition3 when condition1 is false. Otherwise I execute 1A and 1B."). But dependent conditions like that could be an indicator of bad code. – ADTC Sep 10 '14 at 05:15
  • @ADTC There is no side effects when executing a condition. More than one condition could be true and in that case I need to give priority to one condition over another. In this case I give priority to condition1. – M.C. Apr 01 '15 at 01:12

8 Answers8

32
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
}
Community
  • 1
Konrad Morawski
  • 9,737
  • 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
    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
  • Out of respect for Demorgan's Law I'd name that variable neitherCondition1Nor2 – candied_orange Aug 13 '19 at 08:06
10

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

Kieveli
  • 660
  • 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
  • 27
    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
  • 1
    Someone once said to me, "Without full regression tests, you're not refactoring." – Kieveli Dec 11 '13 at 18:40
  • 1
    this is not an answer. "Read the question carefully. What, specifically, is the question asking for? Make sure your answer provides that – or a viable alternative..." ([answer]) – gnat Dec 13 '13 at 14:04
6

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.)

DougM
  • 6,371
4

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;
coutol
  • 157
2

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.

Jerry Coffin
  • 44,495
  • The asker does care about the order of evaluation for the conditions. As mentioned by asker: "I always* need to check condition1 before any other condition."* So this solution cannot be used. – ADTC Sep 10 '14 at 05:27
  • @ADTC: I think you're reading more into his wording than was really intended. If you read through the comments, you'll see that his concern isn't really with the order in which the conditions are evaluated, but with ensuring that statement2 is evaluated only if condition1 is false. My last sentence does cover the other possibility, but his comments made it sound as if that wasn't really the intent. – Jerry Coffin Sep 10 '14 at 05:50
2

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.

jdv-Jan de Vaan
  • 929
0

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.

supercat
  • 8,445
  • 23
  • 28
0

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.

Newtopian
  • 7,201