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

A Unicode fgetc() in PHP

In preparation for a presentation I’m giving at this month’s Syracuse PHP Users Group meeting, I found the need to read in Unicode characters in PHP one at a time. Unicode is still second-class in PHP; PHP6 failed and we have to fallback to extensions like the mbstring extension and/or libraries like Portable UTF-8 . And even with those, I didn’t see a unicode-capable fgetc() so I wrote my own. Years ago, I wrote a post describing how to read Unicode characters in C , so the logic was already familiar. As a refresher, UTF-8 is a multi-byte encoding scheme capable of representing over 2 million characters using 4 bytes or less. The first 128 characters are encoded the same as 7-bit ASCII with 0 as the most-significant bit. The other characters are encoded using multiple bytes, each byte with 1 as the most-significant bit. The bit pattern in the first byte of a multi-byte sequence tells us how many bytes are needed to represent the character. Here’s what the function looks like: f...
1 comment
Read more