why would I refactor this code as Cyclomatic Complexity is 58

Question

I read that having CC 10 or less would be highly maintainable code. But the method that I wrote have CC 58. Thanks to VS 2010 code analysis tool. I believe that the method I wrote is very simple, readable and maintainable as far as my understanding. Hence I would not prefer refactoring the code. But since CC is higher than acceptable, I am wondering why would one refactor this method. I am learning things to improve my code If I have mistake, plese correct me. Here is the code.

private string MapBathRooms(string value)
    {
        double retValue = 0;
        if (value == "1" || value == "One")
            retValue = 1;
        if (value == "OneAndHalf" || value == "1.5" || value == "1 1/2")
            retValue = 1.5;
        if (value == "2" || value == "Two")
            retValue = 2;
        if (value == "TwoAndHalf" || value == "2.5" || value == "2 1/2")
            retValue = 2.5;
        if (value == "3" || value == "Three")
            retValue = 3;
        if (value == "ThreeAndHalf" || value == "3.5" || value == "3 1/2")
            retValue = 3.5;
        if (value == "4" || value == "Four")
            retValue = 4;
        if (value == "FourAndHalf" || value == "4.5" || value == "4 1/2")
            retValue = 4.5;
        if (value == "5" || value == "Five" || value == "FourOrMore")
            retValue = 5;
        if (value == "FiveAndHalf" || value == "5.5" || value == "5 1/2")
            retValue = 5.5;
        if (value == "6" || value == "Six")
            retValue = 6;
        if (value == "SixAndHalf" || value == "6.5" || value == "6 1/2")
            retValue = 6.5;
        if (value == "7" || value == "Seven")
            retValue = 7;
        if (value == "SevenAndHalf" || value == "7.5" || value == "7 1/2")
            retValue = 7.5;
        if (value == "8" || value == "8+" || value == "Eight" || value == "SevenOrMore")
            retValue = 8;
        if (value == "EightAndHalf" || value == "8.5" || value == "8 1/2")
            retValue = 8.5;
        if (value == "9" || value == "Nine")
            retValue = 9;
        if (value == "NineAndHalf" || value == "9.5" || value == "9 1/2")
            retValue = 9.5;
        if(value == "10" || value == "Ten")
            retValue = 10;
        if (value == "TenAndHalf" || value == "10.5" || value == "10 1/2"
            || value == "10+" || value == "MoreThanTen" || value == "11")
            retValue = 10.5;

        if (retValue == 0)
            return value;

        return retValue.ToString();
    }

Answer 1

Why not just have a Dictionary<string, double>? That will make for much simpler code - you've separated the data from the lookup code.

private static readonly Dictionary<string, double> BathRoomMap =
    new Dictionary<string, double>
{
    { "1", 1 },
    { "One", 1 },
    { "OneAndHalf", 1.5 },
    { "1.5", 1.5 },
    { "1 1/2", 1.5 }
    // etc
};

private static string MapBathRooms(string value)
{
    double result;
    if (!BathRoomMap.TryGetValue(value, out result))
    {
        return value; // Lookup failed
    }
    return result.ToString();
}

In fact, you could make it even simpler by avoiding the ToString call - just make it a Dictionary<string, string>:

private static readonly Dictionary<string, string> BathRoomMap =
    new Dictionary<string, string>
{
    // Note: I've removed situations where we'd return the
    // same value anyway... no need to map "1" to "1" etc
    { "One", "1" },
    { "OneAndHalf", "1.5" },
    { "1 1/2", "1.5" }
    // etc
};

private static string MapBathRooms(string value)
{
    string result;
    if (!BathRoomMap.TryGetValue(value, out result))
    {
        return value; // Lookup failed
    }
    return result;
}

As ChrisF says, you could also read this from a file or other resource.

Benefits of doing this:

• It's much easier to avoid mistakes and to extend, IMO. There's a simple 1:1 mapping from input to output, as opposed to logic which could go wrong
• It separates out the data from the logic
• It allows you to load the data from other places if need be.
• Because collection initializers use Dictionary<,>.Add, if you have a duplicate key you'll get an exception when you initialize the type, so you'll spot the error immediately.

Put it this way - would you ever consider refactoring from the Dictionary-based version to the "lots of real code" version? I certainly wouldn't.

If you really, really want to have it all in the method, you could always use a switch statement:

private static string MapBathRooms(string value)
{
    switch (value)
    {
        case "One":
            return "1";
        case "OneAndHalf":
        case "1 1/2":
            return "1.5";
        ...
        default:
            return value;
    }
}

I'd still use the dictionary form myself... but this does have the very slight advantage that duplicate detection is brought forward to compile-time.

Answer 2

I agree with the other posters about using a dictionary for a mapping, but I also want to point out that code like this often has hard to find bugs. For example:

• You convert "FourOrMore" into 5, but "MoreThanTen" is converted into 10.5. This seems inconsistent.
• You convert "11" into 10.5 which also seems inconsistent with the rest of the code.

A general algorithm for doing the conversion might be harder to write initially, but could easily save you time in the long run.