38

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?

  • 1
    Possible duplicate of Keep indentation level low – gnat Jul 25 '17 at 11:46
  • 24
    The key thing here is "Resharper Suggests". Take a look at the suggestion, if it makes the code easier to read and is consistent, use it. It does the same thing with for/for each loops. – Jon Raynor Jul 25 '17 at 14:11
  • I generally demote those resharper hints, I don't always follow, so they don't show up in the side bar anymore. – CodesInChaos Jul 25 '17 at 19:31
  • 1
    ReSharper removed the negative, which is usually a good thing. However, it did not suggest a ternary conditional which could fit nicely here, if the block is really as simple as your example. – dcorking Jul 26 '17 at 8:21
  • 2
    Doesn't ReSharper also suggest to move the conditional into the query? foreach (someObject in someObjectList.Where(object => object != null)) { ... } – Roman Reiner Jul 26 '17 at 16:48
62

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.

  • 64
    I love to see the tide of developer opinion going this way. There's so much nesting in code simply because there of an extraordinary popular delusion that break, continue and multiple return are not structured. – JimmyJames Jul 25 '17 at 13:45
  • 6
    the trouble is break etc are still control flow you have to keep in your head 'this cant be x or it would have exited the loop at the top' might not need further thought if its a null check which would throw an exception anyway. But its not so good when its more nuanced 'only run half this code for this status on thursdays' – Ewan Jul 25 '17 at 15:13
  • 2
    @Ewan: What's the difference between 'this cant be x or it would have exited the loop at the top' and 'this can't be x or it wouldn't have entered this block'? – Collin Jul 25 '17 at 19:41
  • nothing is the complexity and meaning of the x which is important – Ewan Jul 25 '17 at 19:44
  • 4
    @Ewan: I think break/continue/return have a real advantage over an else block. With early get outs, there are 2 blocks: the get-out block and the stay there block; with else there are 3 blocks: if, else, and whatever comes after (which is used whatever the branch taken). I prefer having less blocks. – Matthieu M. Jul 26 '17 at 11:08
33

Don't not avoid negatives. Humans suck as 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 negitive that has burned it's 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.

  • 1
    Well, if you have more code in the loop, how it works is dependant on how it all comes together. It isn't independant code. The refactored code in the question was correct for the example, but may not be correct when not in isolation - was that the point you were going for? (+1 for the don't not avoid negatives though) – Baldrickk Jul 26 '17 at 8:50
  • @Baldrickk if (foo != null){ foo.something() } lets me keep adding code without caring if foo was null. This doesn't. Even if I don't need to do that now I don't feel motivated to screw the future by saying "well they can just refactor it if they need that". Feh. Software is only soft if it's easy to change. – candied_orange Jul 26 '17 at 9:04
  • 4
    "keep adding code without caring" Either the code should be related (otherwise, it should probably be seperate) or there should be care taken to understand the functionality of the function / block you are modifying as adding any code has the potential to change behaviour beyond that intended. For simple cases like this, I don't think it matters either way. For more complex cases, I would expect understanding the code to take precidence, so I would choose whichever I think is most clear, which could be either style. – Baldrickk Jul 26 '17 at 9:26
  • related and intertwined are not the same thing. – candied_orange Jul 26 '17 at 10:13
  • 1
    Don't not avoid negatives :D – VitalyB Jul 28 '17 at 17:50
20

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;
    }
}
  • 11
    Love this line: foreach (subObject in someObjectList.where(i=>i != null)) Don't know why I've never thought of something like that. – Barry Franklin Jul 25 '17 at 12:11
  • @BarryFranklin ReSharper should have a green dotted underline on the foreach with "Convert to LINQ-expression" as the suggestion that would do that for you. Depending on the operation, it might do other linq methods like Sum or Max as well. – Kevin Fee Jul 25 '17 at 16:46
  • @BarryFranklin It's probably a suggestion you want to set at a low level and only use discretionarily. While it works fine for trivial cases like this example I find in more complex code the way it picks more complex conditionals part into bits that are Linqifiable and the remaining body tends to create things that are much harder to understand than the original. I'll generally at least try the suggestion because when it can convert an entire loop into Linq the results are often reasonable; but half and half results tend to get ugly fast IMO. – Dan Neely Jul 25 '17 at 20:06
  • Ironically, the edit by resharper has the exact same level of nesting, because it also has an if followed by a one-liner. – Peter Jul 26 '17 at 9:48
  • heh, no braces though! which is apparently allowed :/ – Ewan Jul 26 '17 at 9:49
6

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.

  • 12
    There shouldn't be enough code inside the loop that you can't see the null-check at the top of it without scrolling. – Bryan Boettcher Jul 25 '17 at 16:27
  • @Bryan Boettcher agreed, but not all code is so well written. And even several lines of complex code might make one forget about (or misreason about) the continue. In practice i'm o.k. with returns cause they form a "clean break", but IMO break and continue lead to confusion and, in most cases, are best avoided. – user949300 Jul 25 '17 at 17:29
  • 4
    @user949300 After ten years of development, I've never found break or continue to cause confusion. I've had them involved in confusion but they've never been the cause. Usually, poor decisions on the developer were the cause, and they would have caused confusion no matter what weapon they used. – corsiKa Jul 25 '17 at 21:47
  • 1
    @corsiKa The Apple SSL bug is actually a goto bug, but could easily have been a break or continue bug. However the code is horrible... – user949300 Jul 25 '17 at 23:31
  • 3
    @BryanBoettcher the problem seems to me that the same devs that think continue or multiple returns are ok also think burying them in large method bodies is also ok. So every experience i have with continue/multiple returns has been in huge messes of code. – Andy Jul 25 '17 at 23:33
2

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.

2

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.

0

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.

0

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');
    ...
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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