Click here to Skip to main content
11,634,796 members (64,532 online)
Click here to Skip to main content

Code Metrics, Code Smells, and Refactoring in Practice

, 25 Feb 2006 MIT 71.1K 56
Rate this:
Please Sign up or sign in to vote.
In this article, I describe how our team uses metrics to identify Code Smells and apply refactorings to correct these Code Smells. This example is real simple, but it shows exactly how some of the eXtreme programming practices are helping our team to maintain our code.

Sample Image - TitleImage600.jpg

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:

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:

CC Value

Risk

1-10

Low risk program

11-20

Moderate risk

21-50

High risk

>50

Most complex and highly unstable method

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:

/// <span class="code-SummaryComment"><summary>
</span>

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:
    /// <span class="code-SummaryComment"><summary>
    </span>
  2. Method to check both parameters are not null and not empty strings:
    /// <span class="code-SummaryComment"><summary>
    </span>
  3. Method to check both parameters aren't populated:
    /// <span class="code-SummaryComment"><summary>
    </span>
  4. Method to check the CustomerID for field length and to pad the parameter to the correct length with zeroes:
    /// <span class="code-SummaryComment"><summary>
    </span>
  5. Method to check ProductNumber for field length and pad the parameter to the correct length with zeroes:
    /// <span class="code-SummaryComment"><summary>
    </span>

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.

License

This article, along with any associated source code and files, is licensed under The MIT License

Share

About the Author

Maruis Marais
Web Developer
New Zealand New Zealand
No Biography provided

You may also be interested in...

Comments and Discussions

 
GeneralMode Code Metric Software Pin
Rich Visotcky1-Mar-06 5:12
memberRich Visotcky1-Mar-06 5:12 
GeneralWrong example! Pin
Pascal Lengelé1-Mar-06 1:20
memberPascal Lengelé1-Mar-06 1:20 
GeneralRe: Wrong example! Pin
Maruis Marais1-Mar-06 10:03
memberMaruis Marais1-Mar-06 10:03 
Hi Pascal,

I want to thank you for the comment. You make some valid points I agree with, but here is some explanations to some of your remarks:

You removed comments on a method instead of improving them
You produced unreadable code: no comment at all, no white line, no code layout at all
You don’t think to “other people” when you scotch the open bracket at the end of a predicate.


The comments, formatting of the code(bracket at end of line), etc. that you found on the refactored code is due to a technical difficulty I had uploading the article, for some reason I was loosing half the method when uploading. I had to shorten the method to be able to upload and I used the above to reduce the length. Confused | :confused:

The other more elementary point I was trying to get across, is to always write your code in an intent revealing fashion. When comparing the original method with the refactored method, without even looking at any of the comments you can read the method names and understand exactly what the method is achieving.

You don’t think OO at all: a method called ValidateRequest with a first parameter called Request should be a method called Validate in the Request object.

Re-factoring is not always dividing a method into multiple methods, it may be performing a code piece in a separated object (a RequestValidator for example).


Agree completely, the above example doesn’t take into effect good encapsulation rules. It is not the job of the web service implementation object to validate the request; it should be the job of the request object to validate itself. But again, the article did not focus just on refactoring or proper OO design. It is more a general XP article with the benefits you will get if you follow some of the practices in your project.

You don’t talk about performances at all, and in this case you call 4 methods with the same parameter (detail? It depends how many times this method is called).


Agreed, I don’t talk about performance. But then performance is normally a task done after development has finished. In the Refactoring book Martin Fowler does allude to this.

I think all your comments are valid, but considering the scope of the article, it would have been 150 pages if I had to go into a lot of detail. I think the message is more about “Hey, look at the following XP practices and here is a concrete example showing you how you will create better, more maintainable code when you employ some of the XP practices”. Is the article perfect? I don’t think so. I could have done a lot more refactoring and structured the code much better, but again you are limited in the amount you can transfer in a four or five page article.

Regards,

Maruis Marais

GeneralRe: Wrong example! Pin
Pascal Lengelé1-Mar-06 22:45
memberPascal Lengelé1-Mar-06 22:45 
GeneralRe: Wrong example! Pin
Maruis Marais1-Mar-06 23:12
memberMaruis Marais1-Mar-06 23:12 
GeneralRe: Wrong example! Pin
Thomas Eyde4-Mar-06 10:47
memberThomas Eyde4-Mar-06 10:47 
GeneralCode smells Pin
cbowl28-Feb-06 10:19
membercbowl28-Feb-06 10:19 
GeneralRe: Code smells Pin
Thomas Eyde4-Mar-06 10:32
memberThomas Eyde4-Mar-06 10:32 
GeneralNice Job Pin
computerguru9238227-Feb-06 14:48
membercomputerguru9238227-Feb-06 14:48 
JokeRe: Nice Job Pin
Ratface2328-Feb-06 21:01
memberRatface2328-Feb-06 21:01 
GeneralRe: Nice Job Pin
Maruis Marais1-Mar-06 10:19
memberMaruis Marais1-Mar-06 10:19 
GeneralRe: Nice Job Pin
computerguru923821-Mar-06 13:06
membercomputerguru923821-Mar-06 13:06 
QuestionWhich software have you used to compute the code metrics? Pin
ugog9127-Feb-06 2:37
memberugog9127-Feb-06 2:37 
AnswerRe: Which software have you used to compute the code metrics? Pin
Maruis Marais27-Feb-06 8:18
memberMaruis Marais27-Feb-06 8:18 
GeneralRe: Which software have you used to compute the code metrics? Pin
Stas Elensky27-Feb-06 21:52
memberStas Elensky27-Feb-06 21:52 
GeneralRe: Which software have you used to compute the code metrics? Pin
computerguru9238228-Feb-06 2:52
membercomputerguru9238228-Feb-06 2:52 
QuestionRe: Which software have you used to compute the code metrics? Pin
ugog912-Mar-06 21:38
memberugog912-Mar-06 21:38 
AnswerRe: Which software have you used to compute the code metrics? Pin
Maruis Marais2-Mar-06 21:57
memberMaruis Marais2-Mar-06 21:57 
GeneralMetrics Pin
mashiharu26-Feb-06 2:48
membermashiharu26-Feb-06 2:48 
GeneralRe: Metrics Pin
Maruis Marais26-Feb-06 9:50
memberMaruis Marais26-Feb-06 9:50 
GeneralRe: Metrics Pin
computerguru9238227-Feb-06 14:51
membercomputerguru9238227-Feb-06 14:51 
JokeRe: Metrics Pin
Ratface2328-Feb-06 21:00
memberRatface2328-Feb-06 21:00 
GeneralRe: Metrics Pin
computerguru923821-Mar-06 16:25
membercomputerguru923821-Mar-06 16:25 
GeneralRe: Metrics Pin
eyesice30-May-12 4:10
membereyesice30-May-12 4:10 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

| Advertise | Privacy | Terms of Use | Mobile
Web03 | 2.8.150728.1 | Last Updated 26 Feb 2006
Article Copyright 2006 by Maruis Marais
Everything else Copyright © CodeProject, 1999-2015
Layout: fixed | fluid