Visual Studio Code Metrics and the Maintainability index of switch case

Question

As a person who loves to follow the best practices,

If i run code metrics (right click on project name in solution explorer and select "Calculate Code Metrics" - Visual Studio 2010) on:

    public static string GetFormFactor(int number)
    {
        string formFactor = string.Empty;
        switch (number)
        {
            case 1:
                formFactor = "Other";
                break;

            case 2:
                formFactor = "SIP";
                break;

            case 3:
                formFactor = "DIP";
                break;

            case 4:
                formFactor = "ZIP";
                break;

            case 5:
                formFactor = "SOJ";
                break;
        }

        return formFactor;
    }

It Gives me a Maintainability index of 61

(of course this is insignificant if you have only this, but if you use an utility like class whos philosophy is doing stuff like that, your utility class will have the maintainability index much worst..)

Whats the solution for this?

Answer 1

First of all: 61 is considered to be maintainable code. For the Maintainability Index, 100 is very easy to maintain code, while 0 is hard to maintain.

• 0-9 = hard to maintain
• 10-19 = moderate to maintain
• 20-100 = good to maintain

The Maintainability Index is based on three code metrics: Namely the Halstead Volumen, the Cyclomatic Complexity and Lines of Code and based on the following formula:

MAX(0,(171 - 5.2 * ln(Halstead Volume) - 0.23 * (Cyclomatic Complexity) - 16.2 * ln(Lines of Code))*100 / 171)

In fact, in your example the root cause for the low value of the Maintainability Index is the Cyclomatic Complexity. This metric is calculated based on the various execution paths in the code. Unfortunately, the metric does not necessarily align with "human readability" of code.

Examples as your code result in very low index values (remember, lower means harder to maintain) but in fact they are very easy to read. This is common when using Cyclomatic Complexity to evaluate code.

Imagine code that has a switch-block for days (Mon-Sun) plus a switch-block for Months (Jan-Dec). This code will be very readable and maintainable but will result in a enormous Cyclomatic Complexity and thus in a very low Maintainability Index in Visual Studio 2010.

This is a well known fact about the metric and should be considered if code is judged based on the figures. Rather than looking at the absolute number, the figures should be monitored over time to understand as an indicator for the change of the code. E.g. if the Maintainability Index was always at 100 and suddenly went down to 10 you should inspect the change that caused this.

Answer 2

The maintainability index could be higher because of the lack of expandability in the method you are choosing for your solution.

The correct solution (Mark Simpson touched on above) is the one that can be expanded to use a new form factor without the code being rebuilt - switch / case statements in code are always a sign that OO design has been forgotten and should always be seen as a bad code smell.

Personally, I would implement...

interface IFormFactor
{
    // some methods
}

class SipFormFactor : IFormFactor...

class DipFormFactor : IFormFactor...

Etc.

...and have the methods on the interface provide the desired functionality - you can think of it [I guess] as being similar in to GoF Command Pattern.

This way your higher level methods can just be...

MyMethod(IFormFactor factor)
{
    // some logic...

    factor.DoSomething();

    // some more logic...
}

...and you can come along at a later date and introduce a new form factor without having to change this code as you would with the hard-coded switch clause. You will also find this approach also lends itself easily to TDD (you should end up with this if you are doing TDD properly) as it is easy for mocking.

Answer 3

I know this answer is very late, but I was interested that no one put up the dictionary solution yet. I've found that when dealing with huge switch statements that are data-oriented like this, it's often more readable to collapse the switch-case into a dictionary.

public static readonly IDictionary<int, string> Factors = new Dictionary<int, string> {
   { 1, "Other" },
   { 2, "SIP" },
   { 3, "DIP" },
   { 4, "ZIP" },
   { 5, "SOJ" }
};

public static string GetFormFactor2(int number)
{
   string formFactor = string.Empty;
   if (Factors.ContainsKey(number)) formFactor = Factors[number];
   return formFactor;
}

This gives you a Maintainability index of 74 - slightly lower than the array solution because of class coupling to a dictionary, but I feel it's more general because it works for any number of types you would normally switch on. Like the array solution, it scales very well and removes a lot of repetitive code.

Generally speaking, using a data-driven approach can help your code to be clearer, because it separates the important pieces (in this case, a condition and a result) from the cruft (in this case, the switch-case).

Answer 4

Two things spring to mind:

Use an enum to marry up the description and value

public enum FormFactor
{
    Other = 1,
    SIP = 2,
    etc.
}

Use a class or struct to represent each form factor

public class FormFactor 
{
    public int Index { get; private set; }
    public string Description { get; private set; }

    public FormFactor(int index, string description)
    {
        // do validation and assign properties
    }
}

Answer 5

I'd do it this way and forget the Maintainability index:

public static string GetFormFactor(int number)
{
    switch (number)
    {
        case 1: return "Other";
        case 2: return "SIP";
        case 3: return "DIP";
        case 4: return "ZIP";
        case 5: return "SOJ";
    }

    return number.ToString();
}

IMHO easy to read and easy to change.

Answer 6

I don't know how much it matters, but the following gets a 76:

private static string[] _formFactors = new[] { null, "Other","SIP","DIP", "ZIP", "SOJ"};
public static string GetFormFactor2(int number)
{
    if (number < 1 || number > _formFactors.Length)
    {
        throw new ArgumentOutOfRangeException("number");
    }

    return _formFactors[number];
}

Answer 7

Clearly for me the Enum method is the most maintainable as it doesn't involve hard-coded strings thus no typo problem and compile-time syntax check. Only restrictions are naming rules.