Always pull out common cases and branch separately?

Question

We had a disagreement in a code review.

What I had written:

if(unimportantThing().isGood && previouslyCalculatedIndex != -1)
{
    //Stuff
}
if(otherThing().isBad && previouslyCalculatedIndex != -1)
{
    //Otherstuff
}
if(yetAnotherThing().isBad)
{
    //Stuffystuff
}

The reviewer called that ugly code. This is what he expected:

if( previouslyCalculatedIndex != -1)
{
    if(unimportantThing().isGood)
    {
        //Stuff
    }
    if(otherThing().isBad)
    {
        //Otherstuff
    }
}
if(yetAnotherThing().isBad)
{
    //Stuffystuff
}

I'd say it's a pretty trivial difference and that the complexity of adding another layer of branching is equivalently bad as one or two logical-ands.

But just to check myself, is this really a grievous coding sin that you would take a firm stance over? Do you always pull out the common cases in your if statements and branch on them separately, or do you add logical-ands to a couple of if statements?

@Giorgio, it's often that -1 is magic for "absence of value." In the days since .NET 2, we have nullables for that. — Anthony Pegram, Feb 19 '13 at 12:43
@Anthony Pegram: OK. I did not know that C# has such a possibility, and I wanted to suggest to use such a type instead of a magic value -1, if possible. — Giorgio, Feb 19 '13 at 14:17
If the library to the hardware we are using returned null, we'd probably use them too. But it returns -1. — Philip, Feb 19 '13 at 15:14

Chewy Gumball · Accepted Answer · 2013-02-18T21:07:20.503

There are two reasons I would prefer the second over the first. The first is repetition: You repeat yourself which can lead to more difficult to maintain code. The second is that I think the second is more clear on when each block of code is executed. If I am debugging and I know that previouslyCalculatedIndex != -1 , then I can immediately skip two whole blocks of code in the second one by just checking one conditional. In the first, I am forced to check two conditionals. Double the work for the same outcome.

Another thing to consider is that if statements are a big place to cause bugs. The longer the conditionals (the more && and || you use), the more chances you have of creating a bug by getting it wrong. Reducing the complexity of your conditionals can make it easier to understand later as well.

Reminds me of all times I've had a NullPointerException in an if statement, and no idea which of the many conditions or chained calls was throwing it! +1 — AlbeyAmakiir, Feb 19 '13 at 05:49

score 16 · Answer 2 · answered Feb 18 '13 at 20:09

While it looks like the two blocks of code are the same, there is the possibility that they are not. In particular, if stuff modifies previouslyCalculatedIndex the code paths for the two may be different.

This is likely not something that is intended to happen, but it could. The first example would expose a bug if the shared conditional variable was to change. The second example prevents this from happening (the conditional is only tested once).

For readability, I find the second example easier to read. I am aware of the scope that the outer if statement has and can read the less complex if statements more easily.

Few rules are 'always' - I doubt that any but the most pedantic always do anything when it comes to code style. However, when possible, one should attempt to code as defensively (writing code that is hard to modify in the wrong way) and understandable as possible.

When things get to the complexity of 5-10 conditions, this is something that needs even more drastic refactoring - extracting methods (both for the conditional and the common code).

Consider how complex the class is when you do get to 5-10 conditions. Additional complexity is correlated to increased bug counts (and more difficult testability).

Take steps early on to keep the complexity low.

I was referring to two `if` statements with two repetitions of checking the index value. 5-10 conditionals is, whoa, yeah, crazy-town. — Philip, Feb 18 '13 at 20:34
@Philip if only I could unsee that which I have seen... if only. — , Feb 18 '13 at 20:43

score 3 · Answer 3 · answered Feb 18 '13 at 20:06

This is pretty minor, but I'd say you're wrong. The main reason why his code is better is because previouslyCalculatedIndex != -1 is only done once. This isn't any kind of performance optimization, but if you ever have to change that condition to say -2 or some such, then you only have to change it in one place instead of two. And, if you continue the pattern (say you had 10 other things which relied on the index being -1), then eventually it would be a huge amount of references to update.

Again, pretty minor, but the reviewer is right. His way is cleaner and has less duplication and will in the long run be easier to maintain. Also, in my opinion, it looks a bit more clear

Also, for your edited in questions(I think). It's an opinion as to if this should be done with 1 if statement (I tend to not), but if you have two, then you do should it like this. What about when you have to go add some feature a year from now and have to add a condition that relies on index being -1. Then, you'll either have to continue the code duplicatation, or take the time to refactor it to how your coworker said it should be in the first place

score 2 · Answer 4 · answered Feb 18 '13 at 22:44

I think context matters. I prefer to allow the important things stand out and they should be foremost in your if tests.

From your example, if the isGood/isBad is what your if-checks are really trying to capture and the previouslyCalculatedIndex is simply a check to make sure you can validly take the appropriate action then I might go with a 3rd approach and put the if(previouslyCalculatedIndex != -1) inside the isGood/isBad braces for each check, since whether you can validly process the isGood/IsBad condition is secondary to what you were really trying to capture (that being isGood/isBad).

Also, what if you want to create a log entry if(previouslyCalculatedIndex == -1) and your isGood/isBad condition was met? None of the other suggestions help with that situation because they are trying to save a line of code instead of making the code more understandable.

I would also change -1 to a const with a meaningful name.

On the other hand, if(previouslyCalculatedIndex != -1) is the important test, then the 2nd option is better.

Always pull out common cases and branch separately?

4 Answers4