perché dovrei refactoring questo codice come complessità ciclomatica è 58

Question

Ho letto che avere CC 10 o meno sarebbe codice altamente mantenibile. Ma il metodo che ho scritto hanno CC 58. Grazie a VS 2010 strumento di analisi del codice. Credo che il metodo che ho scritto è molto semplice, leggibile e mantenibile per quanto riguarda la mia comprensione. Quindi io non preferirei refactoring del codice. Ma dal momento che CC è superiore che accettabile, mi chiedo il motivo per cui si dovrebbe refactoring di questo metodo. Sto imparando cose per migliorare il mio codice Se ho sbagliato, plese mi corregga. Ecco il codice.

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

Perché non basta avere un Dictionary<string, double>? Che farà per molto codice più semplice -. Hai separati Dati dal codice di ricerca

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 realtà, si potrebbe rendere ancora più semplice, evitando la chiamata ToString - basta fare 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;
}

Come ChrisF dice, si potrebbe anche leggere questo da un file o altre risorse.

I vantaggi di fare questo:

• E ' molto più facile evitare errori e di estendere, IMO. C'è una semplice mappatura 1: 1 tra ingresso e uscita, al contrario di logica che potrebbe andare male
• Si separa i dati dalla logica
• Permette di caricare i dati provenienti da altri luoghi se necessario.
• Perché inizializzatori raccolta Destinazione Dictionary<,>.Add, se si dispone di una chiave duplicata si otterrà un'eccezione quando si inizializza il tipo, quindi dovrete individuare l'errore immediatamente.

Mettiamola così - vuoi mai prendere in considerazione la versione refactoring dalla versione basata su dizionario alle "un sacco di codice reale"? Io di certo non lo farei.

Se davvero, vuole veramente avere tutto nel metodo, si può sempre utilizzare un'istruzione 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;
    }
}

mi piacerebbe ancora di utilizzare il modulo dizionario me stesso ... ma questo non ha il leggero vantaggio che il rilevamento duplicato è portato avanti per di compilazione -time.

Answer 2

Sono d'accordo con altri manifesti sull'utilizzo di un dizionario per una mappatura, ma voglio anche sottolineare che il codice come questo ha spesso difficile da trovare bug. Ad esempio:

• Si converte "FourOrMore" in 5, ma "MoreThanTen" viene convertito in 10.5. Questo sembra incoerente.
• Si converte "11" in 10.5, che sembra anche incoerente con il resto del codice.

Un algoritmo generale per fare la conversione potrebbe essere più difficile da scrivere, ma potrebbe facilmente risparmiare tempo nel lungo periodo.

Answer 3

Per rispondere il perché, piuttosto che il come:

Uno dei motivi è quello che ho detto nel mio commento sulla risposta di Jon Skeet, ma utilizzando il dizionario e risorsa esterna consente di modificare il comportamento della vostra applicazione senza dover ricostruire ogni volta i requisiti di cambiamento.

Un'altra è la velocità di esecuzione. Il codice ha per controllare qualche decina di stringhe per trovare il risultato - mentre ci sono modi per fermare l'esecuzione, una volta che hai trovato un match, si devono ancora verificare potenzialmente tutti. Utilizzando un dizionario vi darà tempi di accesso lineari indipendentemente dall'ingresso.

Answer 4

Sì. Molto mantenibile davvero.

Prova questo, invece:

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

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

Tenendo questo in dizionario dovrebbe mantenere la CC ad appena 2. Il dizionario può essere inizializzato con la lettura da file o un'altra risorsa.

CC è (più o meno) il numero dei potenziali percorsi di esecuzione di un metodo. Il vostro CC per questo metodo è che alto, perché non è stato utilizzato una struttura adatta per affrontare questo tipo di problemi (qui: un dizionario). Utilizzando strutture di dati adeguati per risolvere i problemi mantiene il vostro ordine codice e riutilizzabile.

Answer 5

Con il principio di DRY (non ripetere se stessi) è possibile sostituire tutte quelle dichiarazioni if con switch. L'interruttore sarà attuato utilizzando una tabella di hash, così sarà anche più veloce di tutte le dichiarazioni if.

È possibile rimuovere tutti i casi in cui si cattura la rappresentazione numerica del numero, come che è gestito dal fallback.

Non vedo un punto nel convertire la stringa in un numero, e poi di nuovo a una stringa di nuovo. E 'più afficient utilizzare stringhe letterali (in quanto sono pre-generato) che creare la stringa al volo. Inoltre, che il problema elliminates cultura, dove per esempio il valore 9.5 determinerebbe la "9,5" stringa invece "9.5" per alcune culture.

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

Si noti che ho lasciato il caso dei risultati di ingresso "11" nel valore di ritorno di "10.5". Non sono sicuro se questo è un bug, ma è quello che il codice originale fa.

Answer 6

Per la tua domanda di carattere generale, per altri casi che non possono essere riscritta come suggerito da altri soccorritori per questa specifica funzione, v'è una variante di CC che conta una dichiarazione caso come una succursale unica per il fatto che è praticamente lo stesso di linear linee di codice in termini di facilità di comprensione (anche se non per la copertura dei test). Molti strumenti che misura una variante offrirà l'altra. Io suggerisco di usare il caso = 1 variante al posto o, così come quello che si sta utilizzando.