100% code coverage might not be enough

I was recently working on a project where code coverage slipped and we needed to put some effort to catch up by writing few unit tests. During the process, we came across an interesting case which proved us that having a 100% code coverage was not  a sufficient criteria to stop writing unit tests.

Here is the method we were trying to cover:

public Status RetrieveInitialStatusForTicketType(RetrieveInitialStatusForTicketTypeRequest request)
        {
            try
            {
                var userProfile = base.CheckAuthorizationAndReturnValidUserProfile(request);

                var statusesWorkflow = StatusesRepository.RetrieveStatusesWorkflowForTicketType(userProfile, request.TicketTypeId);

                if (statusesWorkflow == null || statusesWorkflow.Where(s => s.IsInitialStatus).Count() != 1)
                    throw new Exception("Must have one and only one initial status");

                var initialStatuses = statusesWorkflow.Where(s => s.IsInitialStatus);
                Status status = initialStatuses.First();
                Status returnedStatus = (Status)status.Clone();
                returnedStatus.PossibleTransitions = null;
                return returnedStatus;
            }
            catch (SecurityException) { throw; }
            catch (InvalidTokenException) { throw; }
            catch (Exception e) { throw new InternalErrorException(e); }
        }

The whole method was covered except the line 10:

throw new Exception("Must have one and only one initial status");

and line 20 that catches the exception.

So we added a new unit test that expected an exception and in which we mocked the StatusesRepository (line 07) to return a null value. We ran the code coverage analyzer and, yeah!!! 100% of the code was covered. We thought we were done… but wait a minute!!!!

The unit testing is not about testing per se, but rather about validating the requirements implemented by a method. In other words, all the requirements the method implements must be validated by one or more unit tests.

However, by looking at our code, we clearly see, at line 09, that the returned workflow must contain one and only one status which flag “IsInitialStatus” is set to true. However, none of our tests was checking for this rule to be implemented although the code was 100% covered.

We decided to write a new unit test in which the workflow returned by the StatusesRepository (line 07) contains more than one initial status and another in which the returned workflow contains no status marked as the initial status. In both cases, the unit test expects and exception to be thrown. Only then will our code be really 100% covered functionaly speaking.

Along the way, we have split the condition of the line 09 into two different if statements in an attempt to make the code more comprehensive (lines 09 and 12).

public Status RetrieveInitialStatusForTicketType(RetrieveInitialStatusForTicketTypeRequest request)
        {
            try
            {
                var userProfile = base.CheckAuthorizationAndReturnValidUserProfile(request);

                var statusesWorkflow = StatusesRepository.RetrieveStatusesWorkflowForTicketType(userProfile, request.TicketTypeId);

                if (statusesWorkflow == null)
                    throw new Exception("No statuses workflow is defined for the specified ticket type id.");

                if (statusesWorkflow.Where(s => s.IsInitialStatus).Count() != 1)
                    throw new Exception("Must have one and only one initial status");

                var initialStatuses = statusesWorkflow.Where(s => s.IsInitialStatus);
                Status status = initialStatuses.First();
                Status returnedStatus = (Status)status.Clone();
                returnedStatus.PossibleTransitions = null;
                return returnedStatus;
            }
            catch (SecurityException) { throw; }
            catch (InvalidTokenException) { throw; }
            catch (Exception e) { throw new InternalErrorException(e); }
        }

Splitting the condition into two separate conditions had two advantages:

1 – Each condition correpsonds to a particular business rule. Line 09 means that a ticket type MUST be associated with a workflow and line 12 expresses the fact that a workflow must have only and only one initial status. It it now clearer that we do have two business rules and that the condition workflow == null was not a simple check to avoid null object exceptions.
2 – Since the two conditions are separated, we could throw a more specific exception in each case that explains more precisely the reason why it has been thrown (which is very helpful when debugging an application).

Conclusion

While writing unit tests, being in TDD or in TTCUBWWL (tests to catch up because we were lazy – to write them before the code -), we must pay attention to the fact that a method must be covered by unit tests that validate  each implemented requirement in the method and bear in mind that reaching a 100% code coverage of a method is not always a sufficient criteria to stop writing tests for the method.

My advise for the team was that they should write their tests before the code; first to avoid another cathing up effort but more importantly, if the developer wrote the code using TDD, he would certainly not miss the unit test to check the method behavior when a workflow has more than one initial status. But this is another debate: the efficiency of TDD in writing clean and more comprehensive code.

Organizing classes in C#

I was doing a quick research on C# coding conventions and I found one rule that comes back in almost all coding conventions I came through: Always put fields (aka private members) at the top of the class definition.

I am not sure of the reasons to do this. Let me explain why I usually advocate the other way around: order properties and methods from the more accessible to the less accessible.

A class has two types of users:

1. developers writing the code of the class (usually one or two developers max),
2. developers who are ‘clients or users’ of the class (the rest of developers and other guys maintaining the software).

I consider that in the total lifecycle of a project, there might be much more developers of the 2nd type than of the first one.

When you use a class or try to maintain it (refactor it or try to find out about what it does to understand the code), you are not interested in private members. You rather focus on public methods and/or properties (concepts of black box that exposes services). Then, if you need to go further, you will probably have a look at the internals of the class. For this reason, I really prefere, when I open a class file, to directly have all interesting information ride away. Just right there and not have to scroll down.

For me this is a consequence of OOP programming and the encapsulation principle.

I would really appreciate comments… Am I the only one who advocates this? It is not a big deal and I gave up on this subject for the moment- however, I generally do not like the argument of ‘This is the standard!’ (which is the only one I got so far). I rather like the real reason underneath it but could not find anything explanation on this rule except a fellow who told me that this was a C++ programmers’ standard who did not have really the choice otherwise the compiler did not compile (I did not verify this argument yet).

C# 3.0 Extension Methods

One of the new features in .NET 3.5 and C# 3.0 is extension methods. Extension methods, as their names state, let you extend any existing type by adding new methods without having to inherit from it.

For instance, one of the common issues we encounter when retrieving data from the database, is to check whether the value is DBNULL or not before we can use it. I used to write a DbReaderHelper class that actually implements ‘dbnull safe’ data retrieval methods. The syntax to declare an extension method is very simple. It is a static method which its first argument starts with the keyword “this” followed by the type we want to extend.

For instance, if I want to add a new method called “nGetInt32” which returns an Int32.MinValue if the field is dbnull otherwise the field value (an Int32), I should write it as follows:

 

public static Int32 nGetInt32(this DbDataReader reader, int index)
{
      if (reader.IsDBNull(index))         
          return Int32.MinValue;      
      return reader.GetInt32(index);
}

How to use it?

The extension must be part of a static class (see the example at the end of this post). To use it, simply add the namespace the class belongs to as part of the “using” directives and that’s it. Even intellisense will take it into account;

 

Why using extension methods

If we want to extend the DataReader class to make it dbnull safe, we would have to inherit from one of its implementations and therefore would not be able to extend all inherited classes. The other advantage is the fact that you can extend any class (even those marked as “sealed”. On the other hand, extending a class has a limitation which is that the extension method can only use public methods and therefore there’s no access to the inner state of the object. 

Methods resolution 

–          Instance methods have priority
–          If the same extension method is declared more than once, the compiler raises an error.
–          The compiler looks into the current namespace and all the namespaces included with the using directive. 

 

C# 3.0 Anonymous types

With C# 3.0 you can declare a variable without typing it explicitly and its type will be inferred based on the right expression. This is different from the non typing in VB like languages (ASP, VB) as the resulting value is really typed and can no longer be assigned to a different type. Also, right after entering the line that declares the variable, intellisense becomes available.

We can see that actually the variable x is of an anonymous type with the two properties Name and EmployeeNumber plus the inherited properties from the Object class. If we declare another anonymous type variable with the same structure: 

var y = new { Name = “Smith”, EmployeeNumber = “199283” }; 

Then the new variable, y, is compatible with x. In another words, since it has the same properties with the same types (string, string) then C# compiler detects that x and y are of the same type even though we never declared it explicitly. In this case the instruction: x  = y is correct and will result in affecting the value of y in x. However if we declare a third variable z:   

var z = new { Name = “Buddy”, Company = “Microsoft” }; 

and try to affect the value of z to x for instance, we will have a compilation error (and not a runtime error – typical for non-typed variables – ). Even though z has two members of string type, all like x and y, it is not compatible. The reason is that the second property has not the same name (company versus EmployeeNumber). 

Some restrictions to anonymous types:

Anonymous types are only permitted for local variables. You cannot declare a class member with the var keyword. A method cannot return a var type neither can it has a var type parameter; all the following declarations won’t compile: 

public int function(var x, int y)
public var function(int x, int y)

however returning a var variable value is permitted:  

public int function(int x, int y)       
{           
  var t = x + y;           
     return t;       
}

The reason is simple : at the point of the ‘return’ statement, the compiler knows that ‘t’ is an integer and can be returned since the method must return an integer. 

Why using anonymous types?

First, let’s calm down the spirits who think that this is a step back to untyped variables. As mentioned in this post, it is really an implicit typing and if you look at it closely, the variables are typed but the type is constructed ‘on the fly’ if we want. We can use it without declaring it. 

This may open the door to poor programming if it is overused. However, the impact might be limited since the anonymous type has only the scope of the method where it is used.  The main advantage of the anonymous types is in LINQ (beyond the scope of this post). I will post something on LINQ pretty quickly… 

Well the only advantage I can see is to be able to manipulate temporary results in a collection of an anonymous structure and then do something with it without having to declare a very lightweight structure or class just to carry the results in one single method. I can see the usage in a data access layer class that has to process the result returned by a DataReader. But until today, we lived without it and never felt really a need for it. 

As a conclusion, I believe that the real advantage is the LINQ query…. Which I will talk about in my next post.