Use Discretion when Reviewing Code Metrics

Code metrics are interesting creatures. Some are just raw numbers, such as depth of inheritance or lines of code, while others are a bit more subjective, like a maintainability index. But ultimately they are all meaningless without broader context and an understanding of the code.

As a brief example, consider the following C# function which accepts a string and returns the 40-character hexadecimal representation of the string's SHA1 hash.

public static String Sha1(String text)
{
    using (SHA1Managed sha1 = new SHA1Managed())
    {
        Byte[] textBytes = Encoding.Unicode.GetBytes(text);
        Byte[] hashBytes = sha1.ComputeHash(textBytes);
        return Convert.ToBase64String(hashBytes);   
    }
}

The function accomplishes one task. Variables are defined closest to their usage. Function calls are clear and do not nest other function calls. Even using ( ) is used so the run-time can automatically dispose of the SHA1Managed resource. Yet a scan using Visual Studio 2010's Code Metrics returns a maintainability index of 71.

The index increases to 73 when the definition of variables move outside of the using ( ) block.

public static String Sha1(String text)
{
    Byte[] textBytes, hashBytes;
    using (SHA1Managed sha1 = new SHA1Managed())
    {
        textBytes = Encoding.Unicode.GetBytes(text);
        hashBytes = sha1.ComputeHash(textBytes);
    }
    return Convert.ToBase64String(hashBytes);  
}

Eliminating the variables altogether will increase the maintainability index to 76.

public static String Sha1(String text)
{
    using (SHA1Managed sha1 = new SHA1Managed())
    {
        return
            Convert.ToBase64String(sha1.ComputeHash(
                Encoding.Unicode.GetBytes(text)));
    }
}

76 is better than 71, but is the latter code really more readable and maintainable?

I posted this example to StackOverflow and asked if anyone knew why the metric would even increase in the first place. The responders agreed it is counter-intuitive, and the consensus is that it is better to focus on writing clean, concise, readable code.

An extremely low-scoring metric can be an indicator that something should be flagged for review, but use your discretion and judgment when reviewing the code. Use the report as a tool to identify possible problems and not a set of requirements to be met.