Inverting an IF statement

Question

So I've been programming for a few years now and recently have started using ReSharper more. One thing that ReSharper always suggests to me is to "invert 'if' statement to reduce nesting".

Let's say I have this code:

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
    }
}

And ReSharper will suggest that I do this:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

It seems that ReSharper will ALWAYS suggest that I invert IFs, no matter how much nesting is going on. This problem is that I kind of like nesting in at least SOME situations. To me is seems easier to read and figure out what is going on in certain places. This is not always the case, but I feel more comfortable nesting sometimes.

My question is: other than personal preference or readability, is there a reason to reduce nesting? Is there a performance difference or something else that I may not be aware of?

Answer 1

It depends. In your example having the non-inverted if statement is not a problem, because the body of the if is very short. However you usually want to invert the statements when the bodies grow.

We are only humans and keeping multiple things at a time in our heads is difficult.

When the body of a statement is large, inverting the statement not only reduces nesting but it also tells the developer they can forget about everything above that statement and focus only on the rest. You are very likely to use the inverted statements to get rid of wrong values as soon as possible. Once you know those are out of the game the only thing that's left are values that can actually be processed.

Answer 2

Don't not avoid negatives. Humans suck at parsing them even when the CPU loves them.

However, respect idioms. We humans are creatures of habit so we prefer well worn paths to direct paths full of weeds.

foo != null has, sadly, become an idiom. I barely notice the separate parts anymore. I look at that and just think, "oh, it's safe to dot off of foo now".

If you want to overturn that (oh, someday, please) you need a really strong case. You need something that will let my eyes effortlessly slide off of it and understand it in an instant.

However, what you gave us was this:

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;
}

Which isn't even structually the same!

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    if(someGlobalObject == null) continue;
    someSideEffectObject = someGlobalObject.SomeProperty;
}

That is not doing what my lazy brain was expecting it to do. I thought I could keep adding independent code but I can't. This makes me think about things I don't want to think about now. You broke my idiom but didn't give me a better one. All because you wanted to protect me from the one negative that has burned its nasty little self into my soul.

So sorry, maybe ReSharper is right to suggest this 90% of the time, but in null checks, I think you've found a corner case where it's best ignored. Tools simply aren't good at teaching you what readable code is. Ask a human. Do a peer review. But don't blindly follow the tool.

Answer 3

It's the old argument about whether a break is worse that nesting.

People seem to accept early return for parameter checks, but its frowned upon in the bulk of a method.

Personally I avoid continue, break, goto etc even if it means more nesting. But in most cases you can avoid both.

foreach (someObject in someObjectList.where(i=>i != null))
{
    someOtherObject = someObject.SomeProperty;
}

Obviously nesting does make things hard to follow when you have many layers

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
            }
        }
    }
}

The worst is when you have both though

foreach (someObject in someObjectList) 
{
    if(someObject != null) 
    {
        someOtherObject = someObject.SomeProperty;
        if(someObject.x > 5) 
        {
            x ++;
            if(someObject.y > 5) 
            {
                x ++;
                return;
            }
            continue;
        }
        someObject.z --;
        if(myFunctionWithSideEffects(someObject) > 6) break;
    }
}

Answer 4

The trouble with the continue is that if you add more lines to the code the logic gets complicated and likely to contain bugs. e.g.

foreach (someObject in someObjectList)
{
    if(someObject == null) continue;
    someOtherObject = someObject.SomeProperty;

    ...  imagine that there is some code here ...

    // added later by a Junior Developer
    doSomeOtherFunction(someObject);
}

Do you want to call doSomeOtherFunction() for all someObject, or only if non-null? With the original code you have the option and it is clearer. With the continue one might forget "oh, yeah, back there we skipped nulls" and you don't even have the option to call it for all someObjects, unless you put that call at the start of the method which may not be possible or correct for other reasons.

Yes, a lot of nesting is ugly and possibly confusing, but in this case I'd use the traditional style.

Bonus question: Why are there nulls in that list to begin with? It might be better to never add a null in the first place, in which case the processing logic is much simpler.

Answer 5

The idea is the early return. Early return in a loop becomes early continue.

Lots of good answers on this question: Should I return from a function early or use an if statement?

Notice that while the question is closed as opinion based, 430+ votes are in favor of early return, ~50 votes are "it depends", and 8 are against early return. So there is an exceptionally strong consensus (either that, or I'm bad at counting, which is plausible).

Personally, if the loop body would be subject to early continue and long enough for it to matter (3-4 lines), I tend to extract the loop body into a function where I use early return instead of early continue.

Answer 6

This technique is useful for certifying pre-conditions when the main bulk of your method occurs when those conditions are met.

We frequently invert ifs at my work and employ a phantom else. It used to be pretty frequent that while reviewing a PR I could point out how my colleague could remove three levels of nesting! It happens less frequently since we do roll out our ifs.

Here is a toy example of something that can be rolled out

function foobar(x, y, z) {
    if (x > 0) {
        if (z != null && isGamma(y)) {
            // ~10 lines of code
            // return something
        }
        else {
           return y
        }
    }
    else {
      return y + z
    }
}

Code like this, where there is ONE interesting case (where the rest are simply returns or small statement blocks) are common. It is trivial to rewrite the above as

function foobar(x, y, z) {
    if (x <=0) {
        return y + z
    }
    if (z == null || !isGamma(y) {
       return y
    }
    // ~ 10 lines of code
    // return something
}

Here, our significant code, the unincluded 10 lines, is not triple-indented in the function. If you have the first style, either you are tempted to refactor the long block into a method or tolerate the indenting. This is where the savings kick in. In one place this is a trivial savings but across many methods in many classes, you save the need to generate an extra function to make the code cleaner and you don't have as much hideous multi-level indentation.

---

In response to OP's code sample, I do concur that that is an overeager collapse. Especially since in 1TB, the style I use, the inversion causes the code to increase in size.

Answer 7

I think the larger point here is that purpose of the loop dictates the best way to organize flow within the loop. If you're filtering out some elements before you loop through the remaining, then continue-ing out early makes sense. However, if you're doing different kinds of processing within a loop, it might make more sense to make that if really obvious, such as:

for(obj in objectList) {
    if(obj == null) continue;
    if(obj.getColour().equals(Color.BLUE)) {
        // Handle blue objects
    } else {
        // Handle non-blue objects
    }
}

Note that in Java Streams or other functional idioms, you can separate out the filtering from the looping, by using:

items.stream()
    .filter(i -> (i != null))
    .filter(i -> i.getColour().equals(Color.BLUE))
    .forEach(i -> {
        // Process blue objects
    });

Incidentally, this is one of the things I love about Perl's inverted if (unless) applied to single statements, which allows the following kind of code, which I find very easy to read:

foreach my $item (@items) {
    next unless defined $item;
    carp "Only blue items are supported! Failed on item $item" 
       unless ($item->get_colour() eq 'blue');
    ...
}

Answer 8

Resharpers suggestions are often useful but should always be evaluated critically. It looks for some pre-defined code patterns and suggest transformations, but it cannot take into account all the factors which make code more or less readable for humans.

All other things being equal, less nesting is preferable, which is why ReSharper will suggest a transformation which reduce nesting. But all other things are not equal: In this case the transformation requires the introduction of a continue, which means the resulting code is actually harder to follow.

In some cases, exchanging a level of nesting for a continue could indeed be an improvement, but this depends on the semantics of the code in question, which is beyond the understanding of ReSharpers mechanical rules.

In your case the ReSharper suggestion would make the code worse. So just ignore it. Or better: Don't use the suggested transformation, but do give a bit of thought to how the code could be improved. Notice that you may be assigning the same variable multiple time, but only the last assignment have effect, since the previous would just be overwritten. This does look like a bug. ReSharpers suggestion does not fix the bug, but it does make it a bit more obvious that some dubious logic is going on.