Home > Best Practices, C#, Never Do That > Scope Nightmare

Scope Nightmare

September 9th, 2009 Leave a comment Go to comments

Today at the workplace I came across a very weird chunk of code. An enormous block of code was wrapped in 25-30 levels of brackets. On a a standard resolution screen (1280×1024) the code was completely out of screen. After spending some time on refactoring it and eliminating annoying scopes I decided to take a screenshot and post it here.
NestedScopes

Below I will describe reasons that can lead to such kind of issues and possible solutions.

  • Nested loops
    When the logic of the algorithm is complicated you may need to iterate over large number of collections. Implementing it all in a single function not only makes the code less readable but also makes it less reusable. In 8 cases from 10 blocks of code from the loops might be needed to be used from other places. The main rule here is to keep the function to have one simple meaning and also not to be too complex. It will also make the whole code easy to maintain in future. Always try to have not more than 3-4 loops in a function. If you need to more – break it apart.
  • Condition branching
    The “if” statement is one of the most commonly used one and inefficient use of it would also lead to unreadable and non-reusable code. Again, try to break blocks into separate functions. There are no specific guidelines. Basically, it’s up to your own professional judgment. However, I follow one practice which helps me a lot.
    The practice says – “Open a scope when you want to emphasize a logical part of the algorithm”. I will explain the meaning on a hands-on example. Consider the following code:

    var obj = LoadObject();

    foreach(var x in obj.Items)
    {
         x.DoSomeThing();
    }

    It gets the instance of the object from LoadObject methods, then iterates of the collection of inner items and does something. Lets assume that this code perfectly fits your business requirements. However, this code is not protected against NullReference exception, as the LoadObject() method may return null. What people usually do is they enclose the foreach loop in if(obj != null) statement – like:

    var obj = LoadObject();

    if (obj != null)
    {
         foreach(var x in obj.Items)
         {
              x.DoSomeThing();
         }
    }

    if you keep adding such kind of protective conditions you will end up with similar or even worse code like:
    NestedScopes
    I’m suggesting to break the execution of the function by return statement, or use the continue to break the loop instead:

    var obj = LoadObject();

    if (obj == null)
         return;

    foreach(var x in obj.Items)
    {
         x.DoSomeThing();
    }

    This will reduce number of scopes and later on you will not pay too much attention to the protective condition. So, open the scopes only when they are a part of the algorithm definition, not the protective/supporting code.

  1. September 25th, 2009 at 12:51 | #1

    A simple technique which is very useful while writing code or refactoring such nightmares šŸ™‚
    I agree with you and think that one of the bad coding practices is having more than two inner brackets. In ideal case we would have only one – however I think it’s just not convenient…

  2. September 27th, 2009 at 11:17 | #2

    Thanks davitb. It makes me happy to know that I’m not the one person in this world to worry about code simplicity and readability. šŸ™‚

  1. No trackbacks yet.

So that we know you're not a robot, please answer the question below: * Time limit is exhausted. Please reload CAPTCHA.