Sunday, July 05, 2009

Round and round (my head spins)

by Patrik Hägne via Legend and truth on 7/5/2009 10:28:00 PM

A year or so ago I ran into a piece of code that made me a little dizzy and nauseous. I've created a piece of code similar to the code in question but the original code was in VB so I can't truly reproduce it but this will have to suffice. Can you figure out what the code does (line 7 in particular)?

public class Foo
{
    private string scoreLabelText;

    public Foo(decimal score)
    {
        score = (Convert.ToInt32((score * 10) / 5) * 5) / 10m;
        this.scoreLabelText = score.ToString();
    }
}

I figured it did pretty much nothing and thought that it had gotten in there by mistake or because some code had changed around it and that it had been significant in the past but wasn’t any more. Normally I would just have deleted it but I was lucky in that I could run a svn-blame and see who wrote it. So I called him and said something like “you’re never gonna be able to talk your way out of this one!”. The thing was, he could, since the code actually does something, it’s just that it’s totally non obvious. What it does is that it rounds the value to the nearest multiple of 0.5. For example…

• 14.234 = 14
• 14.44 = 14.5
• 93.1 = 93
• 93.71 = 93.5

What brought me to writing this blog post was that a guy new to the project ran into this same piece of code this Friday and had the same initial reaction as I did, he wanted to delete the line. That the "algorithm” is actually buggy is beside the point, the point is that code should always express it’s intent. The following would be a lot easier to understand:

public class Foo
{
    private string scoreLabelText;

    public Foo(decimal score)
    {
        var roundedScore = RoundToNearestHalfOrIntegerValue(score);
        this.scoreLabelText = roundedScore.ToString();
    }

    private static decimal RoundToNearestHalfOrIntegerValue(decimal value)
    {
        return (Convert.ToInt32((value * 10) / 5) * 5) / 10m;
    }
}

So just by moving the hard to decipher line into a method of its own we’ve made it a lot easier to understand and it’s also a lot less likely to be changed by someone who doesn't understand what the code should do.

As I said earlier there’s also a bug in the code. The bug is that it will round half way values down sometimes, for example 1.25 will be rounded to 1 instead of 1.5. I consider this a bug in this case since it’s not what’s intended even though it’s not necessarily an invalid way of doing rounding (it’s almost bankers rounding). A more to the point algorithm solves this problem and actually is a little more readable to. Note that this is not the only algorithm I could’ve chosen for this task, it’s just that it’s the one that I think is the most readable in the specific context.

public class Foo
{
    private string scoreLabelText;

    public Foo(decimal score)
    {
        var roundedScore = RoundToNearestHalfOrIntegerValue(score);
        this.scoreLabelText = roundedScore.ToString();
    }

    private static decimal RoundToNearestHalfOrIntegerValue(decimal value)
    {
        return Math.Round(value * 2m) / 2m;
    }
}