¿Por qué refactorizaría este código como complejidad ciclomática es 58

Question

Leí que tener CC 10 o menos sería un código altamente mantenible. Pero el método que escribí tiene CC 58. Gracias a la herramienta de análisis de código VS 2010. Creo que el método que escribí es muy simple, legible y mantenible en cuanto a mi entendimiento. Por lo tanto, no preferiría refactorizar el código. Pero dado que CC es más alto de lo aceptable, me pregunto por qué un refactor de este método. Estoy aprendiendo cosas para mejorar mi código si tengo error, por favor corríjeme. Aquí está el código.

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

¿Por qué no solo tener un Dictionary<string, double>? Que hará mucho Código más simple: ha separado el datos del código de búsqueda.

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();
}

De hecho, podría hacerlo aún más simple evitando la llamada de tostración, solo hágalo un 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;
}

Como dice Chrisf, también podría leer esto desde un archivo u otro recurso.

Beneficios de hacer esto:

• Es mucho Más fácil de evitar errores y extender, en mi opinión. Hay un mapeo simple 1: 1 de la entrada a la salida, en lugar de la lógica que pudo ir mal
• Separa los datos de la lógica
• Le permite cargar los datos de otros lugares si es necesario.
• Porque se usan los inicializadores de la colección Dictionary<,>.Add, si tiene una clave duplicada, obtendrá una excepción cuando inicialice el tipo, por lo que detectará el error de inmediato.

Ponlo de esta manera - ¿Podrías alguna vez ¿Considera la refactorización de la versión basada en el diccionario a la versión "LUNCO CÓDIGO REAL"? Ciertamente no lo haría.

Si realmente, realmente quieres tenerlo todo en el método, siempre puedes usar una instrucción Switch:

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

Todavía usaría la forma del diccionario ... pero esto tiene la ligera ventaja de que la detección duplicada se presenta a compilar-tiempo.

Answer 2

Estoy de acuerdo con los otros carteles sobre el uso de un diccionario para un mapeo, pero también quiero señalar que un código como este a menudo tiene errores difíciles de encontrar. Por ejemplo:

• Convierte "Fourormore" en 5, pero "Morethanten" se convierte en 10.5. Esto parece inconsistente.
• Convierte "11" en 10.5, lo que también parece inconsistente con el resto del código.

Un algoritmo general para hacer la conversión podría ser más difícil de escribir inicialmente, pero podría ahorrarle tiempo fácilmente a largo plazo.

Answer 3

Para responder al por qué en lugar de cómo:

Una razón es la que menciono en mi comentario sobre la respuesta de Jon Skeet, pero el uso del diccionario y el recurso externo le permite modificar el comportamiento de su aplicación sin tener que reconstruirlo cada vez que cambian los requisitos.

Otro es la velocidad de ejecución. Su código tiene que verificar unas pocas docenas de cadenas para encontrar el resultado: si bien hay formas de detener la ejecución una vez que haya encontrado una coincidencia, aún debe verificarlas potencialmente todas. El uso de un diccionario le dará tiempos de acceso lineales independientemente de la entrada.

Answer 4

Sí. Muy mantenible de hecho.

Prueba esto en su lugar:

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

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

Mantener esto en el diccionario debe mantener el CC solo 2. El diccionario puede inicializarse leyéndolo desde el archivo u otro recurso.

CC es (más o menos) el número de posibles rutas de ejecución de un método. Su CC para ese método es tan alto, porque no ha usado una estructura adecuada para lidiar con este tipo de problemas (aquí: un diccionario). El uso de estructuras de datos apropiadas para resolver problemas mantiene su código ordenado y reutilizable.

Answer 5

Con el principio de seco (no se repita) puede reemplazar todos esos if declaraciones con un switch. El interruptor se implementará utilizando una tabla hash, por lo que también será más rápido que todo el if declaraciones.

Puede eliminar todos los casos en los que capta la representación numérica del número, como lo maneja el respaldo.

No veo un punto para convertir la cadena en un número y luego volver a una cadena nuevamente. Es más afirma usar cadenas literal (ya que están pre-creadas) que crear la cuerda sobre la mosca. Además, eso ellimina el problema de la cultura, donde, por ejemplo, el valor 9.5 resultaría en la cadena "9,5" en vez de "9.5" para algunas culturas.

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;
}

Tenga en cuenta que dejé el caso de la entrada "11" resulta en el valor de retorno de "10.5". No estoy seguro de si eso es un error, pero ese es lo que hace el código original.

Answer 6

A su pregunta general, para otros casos que no se pueden refactorizar como lo sugieren otros respondedores para esta función específica, existe una variante de CC que cuenta una declaración de caso como una rama única por razones de que es prácticamente lo mismo que las líneas lineales de Código en facilidad de comprensión (aunque no para la cobertura de prueba). Muchas herramientas que miden una variante ofrecerán la otra. Sugiero usar la variante Case = 1 en su lugar o, así como la que está utilizando.