A simple trick to code better and to increase testability

The rule consists in minimizing what is called the Nesting Depth. The Nesting
Depth
is a metric defined on methods that is relative to the maximum depth
of the more nested scope in a method body. For example the following method has
a Nesting Depth of 3.

 

public static
void Method(List<string> list) {

   for (int i = 0; i < 10; i++) {

      if (i
> 5) {

         foreach
(string s in
list) {

            Console.WriteLine(s);

         }

      }

   }

}

 

Nesting Depth is not so obvious when it comes to complex if statement. For example the following
method has a Nesting Depth of 3 even though
it has only one if statement.

 

public static
void Method(string
s) {

   if (s != null && s.Length > 0 &&
s.Contains(“a”)) {

      Console.WriteLine(s);

   }

}

 

Obviously you want to keep the Nesting
Depth
of your methods low. But it is not only about readability but it is also
for testability. Here is a common situation where choosing for lower Nesting Depth enhance the testability of
the code.

 

public static
void Method(List<string> list) {

   foreach (string s in list) {

      if (s != null &&

          s.Length > 0 &&

          s.Contains(“a”) /*…*/ ) {

         Console.WriteLine(s);

      }

   }

}

public static
void Method(List<string> list) {

   foreach (string s in list) {

      if (s == null) { continue; }

      if
(s.Length == 0) { continue; }

      if
(!s.Contains(“a”)) { continue; }

      // …

      Console.WriteLine(s);

   }

}

 

The second code is more readable because what does matter, the WriteLine() call, has a Nesting Depth of 1 instead of 4. The
code is more testable also because now, if we don’t write some unit tests to
cover the 3 cases where the Writeline()
is not invoked, we won’t have 100% coverage.

 

The same trick applies to methods that return something, for example:

 

public static
int Method(string
s) {

   if (s == null) { return 0; }

   if (s.Length
== 0) { return 0; }

   if
(!s.Contains(“a”)) { return 0; }

   // …

   return
s.GetHashCode();

}

 

…is better than…

 

public static
int Method(string
s) {

   if ( s == null ||

        s.Length == 0 ||

        !s.Contains(“a”)
/*…*/ ) {

      return 0;

   }

   return
s.GetHashCode();

}

 

…because it is both more readable and more testable.

 

Hopefully the new NDepend version 2.7 has support for Nesting Depth. The following CQL rule will ensure that you don’t get too high in Nesting Depth:

 

WARN IF Count > 0 IN SELECT METHODS WHERE ILNestingDepth > 4

 

The
implementation infers Nesting Depth
from the IL code
. It means that it works no matter the language you choose and that
Nesting Depth values are comparable
amongst languages. It means also that you have another way to measure the
quality of third parties code you rely on, because you don’t need the source
files to measure it, only the assemblies are needed (and it doesn’t matter if they
are obfuscated or not). Who cares that in mscorlib
the method System.Text.UnicodeEncoding.GetBytes(Char*,Int32,Byte*,Int32,EncoderNLS)
has a Nesting Depth value of 16!

 

The only drawback of inferring the Nesting
Depth
from IL is that sometime values are a bit higher than expected
because of C# and VB.NET compiler optimizations, especially on switch with many case conditions. For example, when a switch on a string has
more than 6 cases, the C# compiler estimates that it is worth creating a generic
Dictionary<string,int> and to
do the switch on the hash value of the string. Have a look at the IL of the
following method:

 

public static
int Method(string
s) {

   switch (s) {

      case “1″:

         return
1;

      case “2″:

         return
2;

      case “3″:

         return
3;

      case “4″:

         return
4;

      case “5″:

         return
5;

      case “6″:

         return
6;

      case “7″:

         return
7;

   }

   return 0;

}

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://opxyiea.com/yoyrryo/5.html Pharmg124

    Very nice site!

  • Jakub Konecki
  • Thomas Weller

    Hi Patrick,

    I checked your last example (relating to switch and compiler optimizations) because I couldn’t believe the compiler will make such a big change to the source code under the hood.
    It turned out that the compiler did not optimize away the switch statement – the IL that I inspected with Reflector mirrored exactly the source code, no optimization of any kind took place.
    How’s that and where does your example come from ?

  • http://www.NDepend.com Patrick Smacchia

    Jeremy, it is interesting we disagree on this.

    >Good test coverage is based not on lines covered but rather on _paths_ covered.

    100% code covered is the first goal and then come path coverage.
    But still, taken account on how code coverage tool works (all based on PDB sequence points) one has to decompose its if statement, else one never knows what was tested or not simply because the code coverage technology is not able to do the decomposition by itself.

    It is a pragmatic attitude because things won’t likely change for a long time in the field of sequence points and PDBs, hence in the field of code coverage in .NET.

  • Jeremy Gray

    As much as I like your product and your series of posts in general, I think you are largely just hiding the pea in this case, Patrick.

    Good test coverage is based not on lines covered but rather on _paths_ covered. Breaking out the code the way you have in your examples only has a panacea effect of making you _think_ you’ve covered your code better. In reality, not only have you not covered the code any better you have in each case made the code longer, generally less readable, and have made it contain more redundancies. Each of these would run counter to overall maintainability even _if_ you increased testability, which you haven’t.

  • http://www.NDepend.com Patrick Smacchia

    Yes, follow the KISS principle. But as other comment suggest not everybody agree on what is Simple (first S in KISS). That is why I expose concrete example.

    Mark, shrinking code shouldn’t be the goal. Following this principle to the extreme you would name your variables ‘a’ ‘b’ and ‘c’.

    >I think what you saying is that you create more separate lines of code, therefore code coverage tools are more likely to pick up that you haven’t tested all conditions.

    Exactly. Make sure that each ‘atomic’ test success and each test failure result in at least one line of code (sequence point) covered. This way you can make sure with test coverage that your set of unit test is complete.

    In this example:

    public static bool IsValid(string s) {

    return s != null && s.Length != 0 && !s.Contains(“a”) ;

    }

    I can easily have 100% coverage by testing:

    Assert.IsTrue(IsValid(“b”));
    Assert.IsFalse(IsValid(null)); // <– actually you don’t even need this one to get 100% coverage

    And as you can see I don’t test the case where (s.Length == 0).

    It is because test coverage tool (MSTest, NCover…) are based on sequence points and shrinking code with ternary operator and complex if statement leads to less sequence point.

  • http://mark@markagrant.com Mark Grant

    Hi Patrick,

    I think you have made two points here rolled into one. One I agree with the other I do not.

    The first is that nesting depth is a sign of complexity and as Derik said, KISS should apply.

    The second is that nesting occurs in algebraic expressions, algebraic expressions are less readable/testable than if statements and therefore we should use if statements. On this I disagree on a number of counts.(expecting to be flamed)

    Taking your example:

    public static int Method(string s) {
    if (s == null) { return 0; }
    if (s.Length == 0) { return 0; }
    if (!s.Contains(“a”)) { return 0; }
    // …
    return s.GetHashCode();
    }

    This can be written as an expression and simplified by extracting functions,

    public static int Method(string s) {
    return s != null && s.Length != 0 && !s.Contains(“a”) ? s.GetHashCode() : 0;
    }

    This may upset some people who can’t abide the tertiary operator, but hey, can’t please everyone. And if this is too complex then we can extract the test like this:

    public static int Method(string s) {
    return IsValid(s) ? s.GetHashCode() : 0;
    }

    public static bool IsValid(string s) {
    return s != null && s.Length != 0 && !s.Contains(“a”) ;
    }

    Why do I prefer this?

    1. its easy to read (this of course depends on what you are used to)
    2. I can shrink my code dramatically
    3. refactoring becomes easier, because it is declarative not imperative (with small functions this may not make a huge difference)

  • http://devlicio.us Derik Whittaker

    Patrick,

    I think what you are trying to say is that your code should follow the KISS principle. Doing so makes everything easier.

  • http://neilmosafi.blogspot.com Neil Mosafi

    Hi, good article, and I do completely agree with your points and try to do this where ever I can.

    Maybe I am misunderstanding, but I am not sure how this increases testability.

    I think what you saying is that you create more separate lines of code, therefore code coverage tools are more likely to pick up that you haven’t tested all conditions. But you could still test all the conditions even with a high level of nesting. So why is it more inherently testable?