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.

Answer 3

To answer the why rather than the how:

One reason is the one I mention in my comment on Jon Skeet's answer, but using the dictionary and external resource allows you to modify the behaviour of your application without having to rebuild it every time the requirements change.

Another is speed of execution. Your code has to check a few dozen strings to find the result - while there are ways to stop the execution once you've found a match, you still have to potentially check them all. Using a dictionary will give you linear access times regardless of the input.

Answer 4

Yeah. Very maintainable indeed.

Try this instead:

// initialize this somewhere
IDictionary<string, string> mapping;

private string MapBathRooms(string value)
{
  if (mapping.ContainsKey(value))
  {
    return mapping[value];
  }
  return value;
}

Keeping this in dictionary should keep the CC to just 2. The dictionary may be initialized by reading it from file, or another resource.

CC is (pretty much) the number of potential execution paths of a method. Your CC for that method is that high, because you haven't used a structure suitable to deal with this kind of problems (here: a dictionary). Using appropriate data structures to solve problems keeps your code tidy and reusable.

Answer 5

With the principle of DRY (don't repeat yourself) you can replace all those if statements with a switch. The switch will be implemented using a hash table, so it will also be faster than all the if statements.

You can remove all cases where you catch the numeric representation of the number, as that is handled by the fallback.

I don't see a point in converting the string into a number, and then back to a string again. It's more afficient to use literal strings (as they are pre-created) than creating the string on the fly. Also, that elliminates the culture issue, where for example the value 9.5 would result in the string "9,5" instead of "9.5" for some cultures.

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

Note that I left the case of the input "11" results in the return value of "10.5". I'm not sure if that's a bug, but that's what the original code does.

Answer 6

To your general question, for other cases that cannot be refactored as suggested by other responders for this specific function, there is a variant of CC that counts a case statement as a single branch on the grounds that it is virtually the same as linear lines of code in ease of comprehension (though not for test coverage). Many tools that measure one variant will offer the other. I suggest using the case=1 variant instead or as well as the one you are using.