Scope Nightmare

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.

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:

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.