Skip to main content

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.
Labels:

Comments

Post a Comment

Popular posts from this blog

Writing a Minimal PSR-0 Autoloader

An excellent overview of autoloading in PHP and the PSR-0 standard was written by Hari K T over at PHPMaster.com , and it's definitely worth the read. But maybe you don't like some of the bloated, heavier autoloader offerings provided by various PHP frameworks, or maybe you just like to roll your own solutions. Is it possible to roll your own minimal loader and still be compliant? First, let's look at what PSR-0 mandates, taken directly from the standards document on GitHub : A fully-qualified namespace and class must have the following structure \<Vendor Name>\(<Namespace>\)*<Class Name> Each namespace must have a top-level namespace ("Vendor Name"). Each namespace can have as many sub-namespaces as it wishes. Each namespace separator is converted to a DIRECTORY_SEPARATOR when loading from the file system. Each "_" character in the CLASS NAME is converted to a DIRECTORY_SEPARATOR . The "_" character has no special ...
6 comments
Read more

Safely Identify Dependencies for Chrooting

The most difficult part of setting up a chroot environment is identifying dependencies for the programs you want to copy to the jail. For example, to make cp available, not only do you need to copy its binary from /bin and any shared libraries it depends on, but the dependencies can have their own dependencies too that need to be copied. The internet suggests using ldd to list a binary’s dependencies, but that has its own problems. The man page for ldd warns not to use the script for untrusted programs because it works by setting a special environment variable and then executes the program. What’s a security-conscious systems administrator to do? The ldd man page recommends objdump as a safe alternative. objdump outputs information about an object file, including what shared libraries it links against. It doesn’t identify the dependencies’ dependencies, but it’s still a good start because it doesn’t try to execute the target file. We can overcome the dependencies of depende...
Post a Comment
Read more

What's Wrong with OOP

Proponents of Object Oriented Programming feel the paradigm yields code that is better organized, easier to understand and maintain, and reusable. They view procedural programming code as unwieldy spaghetti and embrace OO-centric design patterns as the "right way" to do things. They argue objects are easier to grasp because they model how we view the world. If the popularity of languages like Java and C# is any indication, they may be right. But after almost 20 years of OOP in the mainstream, there's still a large portion of programmers who resist it. If objects truly model the way people think of things in the real world, then why do people have a hard time understanding and working in OOP? I suspect the problem might be the focus on objects instead of actions. If I may quote from Steve Yegge's Execution in the Kingdom of Nouns : Verbs in Javaland are responsible for all the work, but as they are held in contempt by all, no Verb is ever permitted to wander about ...
9 comments
Read more