views:

503

answers:

5

Hi there!

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?

+1  A: 

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
    }
}
Mark Simpson
hmm, i´m with some diffcult to understand your solution. Can you please be a little more specific? A more detailed example?Regards
pee2002
A: 

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];
}
John Saunders
Yes, its a solution, but not much elegant one :P i was thinking in a struct way of doing this. Any ideas?
pee2002
A: 

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.

ondesertverge
Thats the way i did.. But i really want to know if there is another solution
pee2002
I find the array and enum methods mentioned below less readable and less maintainable.
ondesertverge
+4  A: 

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.

aheil
A: 

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.

Jem