Tag: future proof

How and Why to Avoid Excessive Nesting

Background

This probably sounds really nit-picky or OCD, but I think it’s an issue worth addressing. Excessive nesting of logic within code can make things nightmarish to read. Even a few of years ago I never thought anything of this. I mean, how much could it really affect someone reading it? He/she must be a complete newb to not be able to read my logic. Fast forward to a co-op placement where this was more closely moderated by my managers, and I began to pay more attention to it…

Why?

Alright, so all that you know so far about my opinion on this is that excessive nesting bothers me. So far, my mission is accomplished. Everything else is just extra. The first issue with excessive nesting is that it actually makes logic hard to follow. If you’re doing code reviews or revisiting your old code, large methods that have lots of nested if statements and loops actually become a tangled mess of logical workflows. You don’t need to believe me yet, but I’m hoping by the end of this you might change your mind.

The next thing, and it’s related, is that it makes refactoring code quite tricky. If you have lot’s of deeply nested if statements, switching up the behaviour of a function even a little bit could have your mind warping with how to tackle all the logical branches. Have fun. Remember that one monolithic function that nobody wanted to go back and refactor? Well, it turns out you need to pass in another parameter now and handle it in all of your separate logical paths. Hold back the tears when you’re trying to recall the logic once you’re 10+ levels deep into nested if statements.

Another key point I’d like to mention is that, in my opinion, the larger the vertical separation between a conditional check and it’s bodies (i.e. the if block and the else block) the more difficult it becomes to read the code. Of course, this may not be a law or an all-the-time thing, but it’s certainly a decent guideline. Think about it though. If you have an enormous block of code for your if statement body, by the time you finish understanding that, you have to go back up to the if statement condition and invert the whole thing to beign to understand what your else block does.

The Offender

Let’s have a look at some real offensive code. Who knows what it does really… Well, nobody does. Why? Because it’s completely contrived to illustrate my point. And that’s that. Behold the horror!

        private void DoStuff()
        {
            foreach (thing in thisList)
            {
                if (condition1)
                {
                    if (condition2)
                    {
                        DoThis(thing);
                    }
                    else
                    {
                        if (condition3)
                        {
                            continue;
                        }
                        else
                        {
                            if (condition4)
                            {
                                continue;
                            }
                            else
                            {
                                if (condition5)
                                {
                                    if (condition6)
                                    {
                                        continue;
                                    }
                                    else
                                    {
                                        if (condition7)
                                        {
                                            continue;
                                        }
                                        else
                                        {
                                            if (condition8)
                                            {
                                                DoThis(thing);
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
                else
                {
                    DoThis(thing);
                }
            }
        }

Pretty filthy, right? In all honesty, anyone who has worked in production code is guaranteed to have seen code that nests much much deeper… veering right off the developer’s window (and some of us code with multiple monitors). It’s a scary world out there, and this example doesn’t even begin to illustrate how bad it can get. I mean, this particular example actually fit on my narrow blog window.

 

A Better Way?

Well, fixing this type of thing is nice and easy:

        private void DoStuff()
        {
            foreach (thing in thisList)
            {
                if (!condition1)
                {
                    DoThis(thing);
                    continue;
                }

                if (condition2)
                {
                    DoThis(thing);
                    continue;
                }
                
                if (condition3 || condition4 || !condition5 || condition6 || condition7)
                {
                    continue;
                }
                
                if (condition8)
                {
                    DoThis(thing);
                }
            }
        }

That’s so much prettier. So what’d I do there? A handful of techniques:

  • Invert logical blocks if they can reduce your nesting. For condition1, I had an if/else block where DoThis(thing) resided in the bottom else block… farrrrr farrrr away from the check itself. I simply inverted this check and moved the else block up. Of course, I then had to put a continue statement there to go back up to the next iteration.
  • For condition2, by simply placing a continue right after the method call in the body, I was able to completely eliminate the else block and reduce nesting by a whole level. This works well with if/else blocks with returns too.
  • Next up was a whole pile of combinations for checking when I’m not going to be calling DoThis(thing). That reduced nesting by a bajillion levels, approximately.
  • The final block there for condition8 was still necessary. Of course, it could have be written to be the inverse check (so, if NOT condition8) with a continue inside the block, followed by DoThis(thing) outside of the if block. To me this would have been a bit overkill.

 

Did You Catch That?

Something extremely important to remember when changing logical flows like this is that the order you check your conditions is EXTREMELY important. Notice how in my refactored version the condition checks are still in the same order that they originally appeared? This is on purpose.
Consider if I move condition8 up to the if statement that tests condition1 and say if NOT condition1, OR condition8. Now this is technically not equivalent to the initial implementation. Why? Because the initial implementation says that for one of the logical paths that call DoThis(thing) the following must be met:
  • condition1 = true
  • conditon2 = false
  • conditon3 = false
  • condition4 = false
  • condition5 = true
  • conditon6 = false
  • condition7 = false
  • condition8 = true

Thus, by combining the condition8 check with the condition1 check, how have I guaranteed all those other conditions?  Additionally, how do I know that skipping those condition checks (i.e. pretend they are method calls) has not altered state elsewhere in the class? This optimization actually may not make the code incorrect in certain situations (because it really depends what those conditions are), but it’s important to note that the checks would not be equivalent to the original. It’s just something to pay attention to, but who knows, you may even find that you can optimize some of those checks away depending on your situation!

 

Summary

Don’t excessively nest your code because it makes me cry at night.

Keep Your Fields Private

Background

A recent discussion about this spurred a bit of a debate. Ever since I can remember, I’ve always been told to keep fields private. If you want to expose them any more than private you should use a property. Of course, I was defending my stance on this and figured I should see what other people were saying. I turned to the trusty Stack Overflow (SO) for some answers.

Argument For: Control

One of the major categories of arguments for keeping fields private has to do with control. Some great points brought up by SO posters included that properties allow you to easily add verification to your data storage. With a field inside of your class, your control is over how this variable is manipulated is constrained to the class. As soon as you expose something outside of the class, you lose this control. Unless of course… you use a property to control the accessibility of this data.

Others added that without a property, there’s no easy way to know when a field changes. From the scope of your parent class, how do you know when a child class modifies your field? This is even more crucial when dealing with events–What if one day you want an event that fires when the value is changed?

One quote I really like from this post:

If now, or at some point in the future, any of your code ever depends on a field some certain value, you now have to add validity checks and fallback logic in case it’s not the expected value – every place you use it. That’s a huge amount of wasted effort when you could’ve just made it a damn property instead 😉

I don’t think I could state it any better.

Argument For: Abstraction

Another major point covered by most posters is the idea of an extensible API–introducing levels of abstraction. Specifically, that you should not neglect your child classes are essentially using an API when they inherit and have access to members that are more than private. There was even a comment describing it as fields “hurting opacity”, which is essentially the exact opposite of providing abstraction. The consumer of the API can see exactly how some data storage was implemented, and perhaps this is something you do want to protect.

One of the greatest points about abstraction that was mentioned is that by using a property, you can change the underlying implementation in the parent class without causing massive headaches in all the child classes. Using a concrete example, perhaps you used a property of type IList to wrap your Array field in your parent class. After your code is out and being used by all of your colleagues who have built child classes on top of your base class, you decide you could really benefit from switching to a concrete list class in your base implementation. Guess what? You’re in luck. All you need to do is modify the base class.

Using my above example, if you had used a protected field, you’d be exposing an array to everyone. You know what’s great about arrays? They use “Length” instead of “Count” for checking the number of size of the collection. If you switched your array field to be a List, you’d now be breaking all the code that was checking the length property of the array. Great.

Other Arguments For

I didn’t feel these arguments were necessarily core to my perspective on the matter, but I thought they were worth mentioning:

  • There are plenty of tools out there that use reflection to check properties of classes (i.e. for things like serialization) that don’t operate on fields.
  • “It’s so easy that there really isn’t a good excuse to not do it”. (I just thought this quote on the matter was kind of funny)

 

Summary

I felt that I had enough re-assurance from the rest of the C# community that this was the accepted practice. There are plenty of good reasons that clearly offer up some great benefits. And the drawbacks? The negligible performance overhead? The “code bloat”? Sure these for-arguments don’t apply in all scenarios (and this post does do an excellent job in offering the against-perspective) but in my opinion making it a “best practice” is setting yourself and your code up for success.

References


  • Nick Cosentino

    Nick Cosentino

    I work as a team lead of software engineering at Magnet Forensics (http://www.magnetforensics.com). I'm into powerlifting, bodybuilding, and blogging about leadership/development topics over at http://www.devleader.ca.

    Verified Services

    View Full Profile →

  • Copyright © 1996-2010 Dev Leader. All rights reserved.
    Jarrah theme by Templates Next | Powered by WordPress