Code Metrics, Code Smells, and Refactoring in Practice

Introduction

Our team uses eXtreme programming practices to manage development on a mission critical system for a large retail chain. We have not adopted all the practices, but use the majority of the practices.

Here is a list of the practices we use:

• Test-Driven Development (VSTS Testing Tools)
• Collective Code Ownership
• Coding Standards (FxCop, Code Metrics)
• Continuous Integration (daily builds, but we are looking to implement a CI environment soon)
• Planning Game (iteration planning and daily standup meetings)
• Customer Test (Fit, VSTS Web Tests)
• Small Releases (fortnightly UAT releases)

Every morning at the standup meeting, the team lead will report on the nightly integration build.

This report includes the following metrics:

• Unit tests passed, failed, ignored
• Fit test passed, failed, ignored, exceptions
• Test coverage (should be more than 80%)
• Cyclomatic complexity (should be less than 10)
• Code instructions (should be less than a 100)
• FxCop rule validations

Each day, metrics are compared to the previous day, and the project manager tracks these metrics to get an overall feel for the health of the project.

Background

The component where this example code was found is a business process web service in a Service Oriented application. The component had, on average, a Cyclomatic Complexity of 8, and this was constant over a couple of weeks. Then suddenly, the complexity of the component jumped to 17. See figure 1 showing the code metrics.

What is Cyclomatic Complexity?

The following good introduction was done by Samudra Gupta on Java Boutique. This metric was introduced by Thomas McCabe and measures the structural complexity of a method.

Suppose you've got a particular implementation class that's become huge in size or too complex to maintain. Or, you've got a single class acting as the control class for a whole business layer, but there's too much business logic embedded within that one class. Or suppose again that you've been handed a class containing too much code duplication. These situations are what are referred to as "complexity."

Learning how to use these three metrics may help steer you towards the correct re-factoring steps:

• Cyclomatic Complexity
• Response for Class
• Weighted methods per class

Cyclomatic Complexity (CC) = number of decision points + 1

Decision points are conditional statements such as if/else, while etc. Consider the following example:

public void isValidSearchCriteria( SearchCriteria s )
{
    if( s != null )
    {
        return true;
    }
    else
    {
        return false;
    }
}

In the above example, CC=2+1=3 (one if + one else + 1).

Cyclomatic complexity has enormous impact on the testability and maintainability of code. As the example shows, if you want to test the isValidSearchCriteria() method, you have to write three tests to verify it: one for the method itself, and two for the decision points within the method. Clearly, if the CC value increases, and there is an increasing number of decision points within any method, it means more effort to test the method.

The following table summarizes the impact of CC values in terms of testing and maintenance of the code:

Identifying Code Problems

Now, let's get back to our method with the moderate risk complexity. At this point, I would like to mention a few things you should think about when you look over the suspect method:

• Code Intent and clarity � How easy it is to understand and figure out what the method is supposed to do.
• Code Smells - Identify common design problems in object-oriented code.

Here is the method:

/// <summary>

/// Validate UI Request and throw a business exception if there is any error

/// </summary>

/// <param name="request">The business process request.</param>

/// <param name="customerIDFieldLength">Length of the Customer field.</param>

/// <param name="productFieldLength">Length of the product field.</param>

/// <returns>Returns true if the Request was validated, else false.</returns>

private bool ValidateRequest( CustomerInquiryRequest request, 
        int customerIDFieldLength, int productFieldLength )
{
    if( request.CustomerProduct.ProductNumber != null && 
        request.Customer.CustomerID != null )
    {
        if( request.CustomerProduct.ProductNumber != string.Empty &&
            request.Customer.CustomerID != string.Empty ){
            // Both were populated

            throw new BusinessException( 
                  HandledErrors.InvalidBothParameterMessage );
        }
        else if( request.Customer.CustomerID == string.Empty && 
            request.CustomerProduct.ProductNumber == string.Empty ) {
            // if objects were instantiated but not populated

            throw new BusinessException( 
                  HandledErrors.CustomerEmptyMessage );
        }
        else if( request.Customer.CustomerID != string.Empty ){
            // Note: ProductNumber was equal

            // to string.empty Check Customer ID length

            if( request.Customer.CustomerID.Length > 
                             customerIDFieldLength ){
                throw new BusinessException( 
                      HandledErrors.CustomerInvalidLengthMessage );
            }
        }else{
            // Note: CustomerID was equal

            // to string.empty check Product Length

            if( request.CustomerProduct.ProductNumber.Length > 
                                          productFieldLength ){
                throw new BusinessException(
                      HandledErrors.ProductInvalidLengthMessage );
            }
        }
    }else if( request.CustomerProduct.ProductNumber == null && 
        request.Customer.CustomerID == null ){
        // Both were null

        throw new BusinessException( HandledErrors.CustomerEmptyMessage );
    }else if( request.CustomerProduct.ProductNumber == null ){
        // ProductNumber was null and CustomerID was not null

        if( request.Customer.CustomerID.Length > 
                         customerIDFieldLength ){
            throw new BusinessException(
                  HandledErrors.CustomerInvalidLengthMessage );
        }
    }else{ // ProductNumber not null and CustomerID was null

        // Check Product Length

        if( request.CustomerProduct.ProductNumber.Length > 
                                      productFieldLength ) {
            throw new BusinessException(
                  HandledErrors.ProductInvalidLengthMessage );
        }
    }
    // Set objects below with formatted data i.e PadLeft

    if( request.Customer.CustomerID != null ){
        request.Customer.CustomerID = request.Customer.CustomerID.PadLeft(
            customerIDFieldLength, 
            Convert.ToChar( "0", CultureInfo.CurrentCulture ) );
    }
    if( request.CustomerProduct.ProductNumber != null ){
        request.CustomerProduct.ProductNumber =
            request.CustomerProduct.ProductNumber.PadLeft(
            productFieldLength, 
            Convert.ToChar( "0", CultureInfo.CurrentCulture ) );
    }
    return true;
}

The first things I found when I looked at this method were the following code smells:

• Long Method - The longer the method, the harder it is to see what it's doing. At the moment, my personal preference is to have methods that are not longer than ten lines of code. If it exceeds ten lines, I'll rather refactor and break the method up.
• Duplicated Code.
• Comments - Should only be used to clarify "why" not "what". Comments can quickly become verbose and reduce code clarity.
• Dead Code - A variable, parameter, method, code fragment, class, etc. that is not used anywhere.

I was also very puzzled about what the method was doing. What verification was taking place here? The intent of what the code was doing wasn't clear. After a good couple of minutes, I figured out that the request passed from the UI to the business process web service had two parameters that needed to be checked.

The following rules where established:

1. If both where null or an empty string, then throw a business exception.
2. If both where populated, throw a business exception.
3. If only the Customer ID is populated, then check if the parameter maximum length isn't violated.
4. If only the Product Number is populated, then check if the parameter maximum length isn't violated.
5. The last thing I found was that the method returned a boolean, but it was impossible to return a false value, so returning anything is redundant.

Solving the Problems Identified

To solve the problems with the suspect method, I've mainly used the extract method as a refactoring, and have applied some new features available in .NET 2.0 to reduce the need to check for null and empty string.

Here is the refactored code:

1. The main ValidateRequest method:

   /// <summary>
   
   /// Validates the customer inquiry request.
   
   /// </summary>
   
   /// <param name="request">The customer inquiry request.</param>
   
   /// <param name="customerFieldLength">Length of the customer field.</param>
   
   /// <param name="productFieldLength">Length of the product field.</param>
   
   private void ValidateRequest( CustomerInquiryRequest request, 
                                 int customerFieldLength,
       int productFieldLength )
   {
       // 1. Check both parameters are not null and not empty strings
   
       CheckCustomerInquiryNotNullOrEmpty( request );
       // 2. Check both parameters aren't populated
   
       CheckCustomerInquiryNullOrEmpty( request );
       // 3. Check CustomerID for field length and pad the parameter
   
       CheckCustomerIDValid( request, customerFieldLength );
       // 4. Check ProductNumber for field length and pad the parameter
   
       CheckProductNumberValid( request, productFieldLength );
   }

2. Method to check both parameters are not null and not empty strings:

   /// <summary>
   
   /// Checks the customer inquiry not null or empty.
   
   /// </summary>
   
   /// <param name="request"> customer inquiry request.</param>
   
   /// <remarks>If both customer id and product number is not null, then 
   
   /// we throw a customer empty message business exception.</remarks>
   
   private void CheckCustomerInquiryNotNullOrEmpty(
                    CustomerInquiryRequest request )
   {
       // Check both parameters are not null or empty string 
   
       if( !string.IsNullOrEmpty( request.CustomerProduct.ProductNumber ) &&
           !string.IsNullOrEmpty( request.Customer.CustomerID ) )
       {
           // Both were populated
   
           throw new BusinessException(
                 HandledErrors.InvalidBothParameterMessage );
       }
   }

3. Method to check both parameters aren't populated:

   /// <summary>
   
   /// Checks the customer inquiry null or empty.
   
   /// </summary>
   
   /// <param name="request">The customer inquiry request.</param>
   
   /// <remarks>
   
   /// If both customer id and product number is null, then we 
   
   /// throw a customer empty message business exception.</remarks>
   
   private void CheckCustomerInquiryNullOrEmpty(
                 CustomerInquiryRequest request )
   {
       if( string.IsNullOrEmpty( request.Customer.CustomerID ) &&
           string.IsNullOrEmpty( request.CustomerProduct.ProductNumber ) )
       {
           // Both are null or empty string
   
           throw new BusinessException(
                 HandledErrors.CustomerEmptyMessage );
       }
   }

4. Method to check the CustomerID for field length and to pad the parameter to the correct length with zeroes:

   /// <summary>
   
   /// Checks the customer ID valid.
   
   /// </summary>
   
   /// <param name="request">The customer inquiry request.</param>
   
   /// <param name="customerIDFieldLength">Length of 
   
   ///            the customer ID field.</param>
   
   /// <remarks>Due to the Product number being null,
   
   ///        we are working with the customer id</remarks>
   
   private void CheckCustomerIDValid( CustomerInquiryRequest request, 
                                      int customerIDFieldLength )
   {    
       if( string.IsNullOrEmpty( request.Customer.CustomerID ) )
       { 
           // Check Customer ID length
   
           if( request.Customer.CustomerID.Length > 
                            customerIDFieldLength )
           {
               throw new BusinessException(
                     HandledErrors.CustomerInvalidLengthMessage );
           }
           
           // Pad the left of the customer id
   
           request.Customer.CustomerID = 
                   request.Customer.CustomerID.PadLeft(
                   customerIDFieldLength, 
                   Convert.ToChar( "0", CultureInfo.CurrentCulture ) );
       }
   }

5. Method to check ProductNumber for field length and pad the parameter to the correct length with zeroes:

   /// <summary>
   
   /// Checks the product number valid.
   
   /// </summary>
   
   /// <param name="request">The  customer inquiry request.</param>
   
   /// <param name="productFieldLength">Length of 
   
   ///              the product field.</param>
   
   /// <remarks>Due to the customer id being null,
   
   ///    we are working with the product number</remarks>
   
   private void CheckProductNumberValid( CustomerInquiryRequest request, 
                                         int productFieldLength )
   {    
       if( string.IsNullOrEmpty( request.CustomerProduct.ProductNumber ) )
       { 
           // Check Product Length
   
           if( request.CustomerProduct.ProductNumber.Length > 
                                         productFieldLength )
           {
               throw new BusinessException( 
                     HandledErrors.ProductInvalidLengthMessage );
           }
   
           // Pad the left of the product number 
   
           request.CustomerProduct.ProductNumber = 
             request.CustomerProduct.ProductNumber.PadLeft(
             productFieldLength, 
             Convert.ToChar( "0", CultureInfo.CurrentCulture ) );
       }
   }

Points of Interest

One of the first things you'll notice when you look at the refactored ValidateRequest method, is the ease with which you can read and understand what the method is trying to achieve by reading just the method names. Writing self documenting and easily readable code isn't hard to do, it just requires dedication.

What you will find upon doing it is that:

1. Other people will enjoy working with your code more
2. It will make you look more professional since it seems you care about others touching the code
3. It will help you write better code since it forces you to clearly define what you are doing

The next point to note is that all the methods are small and real easy to understand. Due to this, everyone on the project can isolate bugs and extend functionality easily.

Testability is also improved due to the reduced complexity. We might now have five methods in the refactored code compared to the one method before, but the highest complexity is now 4. This helps us to isolate each code path easier, and reduces the risk of untested code, which normally helps increase test code coverage on the component.

Conclusion

I think the benefits of using eXtreme Programming practices are really unmistakable. Probably, the most beneficial is the constant feedback and communication between everyone on the project. Other practices like Fit and Test-Driven Development, Refactoring, and Continuous Integration are paramount to keeping the quality of the code up to the highest standard.

Probably, if you look at this specific example, if we didn't have the feedback loop happening every day at our stand-up meeting with the code metrics report about the health of the code, problems like the one above would just fall through the cracks and technical debt would build up, until the code base has grown so big and fragile that nobody would ever want to change anything.

References

1. Measuring the Complexity of OO Systems by Samudra Gupta.
2. Refactoring: Improving the Design of Existing Code by Martin Fowler.
3. Test Driven Development: By Example by Kent Beck.
4. Fit for Developing Software: Framework for Integrated Tests by Rick Mugridge, Ward Cunningham.