Improving code quality using NDepend

We are often too liberal adding project references. In our team I’ve sent once a warning that every time an assembly reference is added to a project, an angel gets killed, but Norway does not seem to be very religious country. Recently we’ve even had a problem with an assembly loaded in two different contexts (using Load and LoadFrom), so it was definitely time to have a closer look at our dependencies. And NDepend is invaluable tool when it comes to dependency analysis.

I haven’t had much of experience with using NDepend, so I decided to gain it step by step and started with metrics for a small solution that consists of our common utility libraries. For dependency analysis purpose there is not much value in running NDepend on a set of utility classes without including code that uses those classes. Utility classes often have low cohesion and mostly reference only .NET FX types. But I wanted to test NDepend’s code quality metrics and perhaps even improve some of our old code. It worked out and I even found and fixed a bug.

But first I installed NDepend that added a new add-in to Visual Studio. Now I could activate NDepend by choosing “Go to VisualNDepend” from project-level context menu.

NDepend generates its metrics for projects included in a Visual Studio solution, and to generate useful metrics it’s important to make sure it works on a right project set (you can always exclude projects from analysis, as I did with unit and integration test projects). Then I started NDepend analysis and it responded soon with a picture familiar to anyone who used the product:

NDepend01

The actual dependcy graph was not very interesting for me at this point. Here is how it looked:

NDepend02

Again, these are just utility classes (split between 4 assemblies: Lfx.Core, Lfx.Data, Lfx.Global and Lfx.Services). NDepend also produced a table that showed number of dependent methods:

NDepend03

Here numbers with green background show the number of methods in the referencing assembly (specified in rows) that use methods from a referenced assembly (specified in columns), and numbers with blue background show the number of referenced methods in the referenced assembly. For example, the table above shows that there 8 methods in Lfx.Global assembly that reference 3 methods in Lfx.Data assembly. By left-clicking on cell I could generate a graph showing related methods:

NDepend04

I postponed more complex dependency analysis until the next time and looked at NDepend code metrics. NDepend comes with more than hundred customizable queries that check various aspects of code quality. For example, this result list displays 10 candidate methods for refactoring:

NDepend05

And here’s the criteria that was used in a CQL query to generate the result set above:

NbLinesOfCode > 30 OR
NbILInstructions > 200 OR
CyclomaticComplexity > 20 OR
ILCyclomaticComplexity > 50 OR
ILNestingDepth > 4 OR
NbParameters > 5 OR
NbVariables > 8 OR
NbOverloads > 6

Query parameters can be easily altered: you just place a cursor on a query parameter and NDepend displays a tooltip with a control to change it:

NDepend06

To make query results even more visual, NDepend highlights the areas that contain the offensive code:

NDepend07

I browsed metrics results and found two methods with high cyclomatic complexity (22 with recommended value not to exceed 15). This surprised me, because I did not expect utility classes to have methods with such complex logic. I checked one of them, it was an overriden Equals operator that tested two instances of the same class for equality. The respective class (called CpType) had several string field members, and the equality check consisted of several similarly looking lines of code:

public override bool Equals(object obj)
{
    CpType type = obj as CpType;
    if (type != null)
    {
        return (string.IsNullOrEmpty(this.FieldA) && string.IsNullOrEmpty(type.FieldA) || this.CodeBase.Equals(type.FieldA)) &&
            (string.IsNullOrEmpty(this.FieldB) && string.IsNullOrEmpty(type.FieldB) || this.TypeName.Equals(type.FieldB)) &&
            /* some lines skipped */
            (string.IsNullOrEmpty(this.FieldZ) && string.IsNullOrEmpty(type.FieldZ) || this.RemoteServer.Equals(type.FieldZ));
    }
    else
    {
        return false;
    }
}

It was not surprising that this code had high cyclomatic complexity: each of the following expressions increases it: if | while | for | foreach | case | default | continue | goto | && | || | catch | ternary operator ?: | ??. But how this can be avoided if a class has a large number of fields and each field should be compared in Equals method?

First, the number of fields is itself a concern. The class above was a collection of parameters used in dynamic type instantiation. It was so old that it even included parameters used to create an instance of remote DCOM server, and at the same time it was recently updated to support instantiation of WCF services. It was a good candidate not only for refactoring – for rewrite, and one of NDepend metrics brought an attention to it. But that was not all. When inspecting the Equals method code, I saw that one of similarly looking lines had a typo: string.IsNullOrEmpty(type.FieldM) instead of string.IsNullOrEmpty(type.FieldN). I must have pasted a previous line and haven’t propertly updated it. And that line with type passed all unit test, so I had another problem: insufficient number of unit tests (in fact I did not have negative unit tests for Equals method).

Following TDD practice, I first created failing unit tests, fixed them and then looked if I could reduce cyclomatic complexity of the method without class rewrite. It was possible if I created a lambda-expression encapsulating field equality test and called it for each field:

public override bool Equals(object obj)
{
    CpType type = obj as CpType;
    if (type != null)
    {
        Func Equals = (x, y) => { return string.IsNullOrEmpty(x) && string.IsNullOrEmpty(y) || string.Equals(x, y); };
        return (Equals(this.FieldA, type.FieldA)) &&
            (Equals(this.FieldB, type.FieldB)) &&
            /* some lines skipped */
            (Equals(this.FieldZ, type.FieldZ));
    }
    else
    {
        return false;
    }
}

I recompiled the code and updated NDepend metrics: it no longer complained about this method. But the type is now in my refactoring TODO list.

While NDepend demonstrated its excellent quality to analyze individual classes and methods, its real value is in dependency analysis. This is what I plan to to next and describe in one of my future posts.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s