Cyclomaticの複雑さが58であるため、なぜこのコードをリファクタリングするのですか

StackOverflow https://stackoverflow.com/questions/7814401

  •  26-10-2019
  •  | 
  •  

質問

CC 10以下が非常に保守可能なコードを使用することは、非常に保守可能なコードを持っていることを読みました。しかし、私が書いた方法にはCC58があります。VS2010コード分析ツールのおかげです。私が書いた方法は、私の理解に関しては非常にシンプルで、読みやすく、保守可能だと思います。したがって、私はコードのリファクタリングを好まないでしょう。しかし、CCは受け入れられるよりも高いため、なぜこの方法をリファクタリングするのか疑問に思っています。間違いがある場合は、コードを改善することを学んでいます。これがコードです。 

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();
    }
役に立ちましたか？

解決

なぜ持っていないのですか Dictionary<string, double>？それは作ります 多くの よりシンプルなコード - あなたは分離しました データ ルックアップコードから。

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

実際、Tostringコールを避けることで、さらに簡単にすることができます - ただそれを作るだけです 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;
}

Chrisfが言うように、ファイルや他のリソースからこれを読むこともできます。

これを行うことの利点：

  • これは 多くの 間違いを避け、拡張するのが簡単です、IMO。ロジックとは対照的に、入力から出力への単純な1：1マッピングがあります。 できる うまくいかない
  • データをロジックから分離します
  • 必要に応じて、他の場所からデータをロードできます。
  • コレクション初期ザーが使用しているためです Dictionary<,>.Add, 、キーが重複している場合は、タイプを初期化するときに例外が表示されるため、すぐにエラーが発見されます。

このように言ってください - あなたはそうしますか これまで 辞書ベースのバージョンから「たくさんの実際のコード」バージョンにリファクタリングすることを検討してください。私は確かにしません。

本当に、本当にすべてをメソッドに入れたい場合は、常にスイッチステートメントを使用できます。

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

私はまだ辞書フォームを自分で使用しています...しかし、これには、重複検出が提出されるという非常にわずかな利点があります コンパイル-時間。

他のヒント

マッピングに辞書を使用することについて他のポスターに同意しますが、このようなコードはバグを見つけるのが難しいことが多いことを指摘したいと思います。例えば：

  • 「fourormore」を5に変換しますが、「モレタンテン」は10.5に変換されます。これは一貫性がないようです。
  • 「11」を10.5に変換しますが、これはコードの残りの部分とも矛盾しているようです。

変換を行うための一般的なアルゴリズムは、最初は書き込むのが難しいかもしれませんが、長期的には簡単に時間を節約できます。

方法ではなく、理由に答えるには：

1つの理由は、Jon Skeetの回答についてコメントしていることですが、辞書と外部リソースを使用すると、要件が変更されるたびにアプリケーションの動作を変更できます。

もう1つは実行速度です。コードは数十文字列をチェックして結果を見つける必要があります。試合を見つけたら実行を停止する方法はありますが、潜在的にすべてをチェックする必要があります。辞書を使用すると、入力に関係なく線形アクセス時間が得られます。

うん。確かに非常に維持可能です。

代わりにこれを試してください：

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

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

これを辞書に保持することで、CCを2にだけ保持する必要があります。ファイルまたは別のリソースから読み取ることにより、辞書は初期化できます。

CCは（ほとんど）メソッドの潜在的な実行パスの数です。この方法のCCは、この種の問題に対処するのに適した構造を使用していないためです（ここ：辞書）。適切なデータ構造を使用して問題を解決すると、コードが整頓され、再利用可能になります。

乾燥の原則で（繰り返さないでください）、あなたはそれらすべてを置き換えることができます if でのステートメント switch. 。スイッチはハッシュテーブルを使用して実装されるため、すべてのものよりも高速になります if ステートメント。

フォールバックによって処理されるため、数の数値表現をキャッチするすべてのケースを削除できます。

文字列を数値に変換するポイントはありません。そして、再び文字列に戻ることができません。その場で文字列を作成するよりも（それらが事前に作成されているように）文字通りの文字列を使用する方が親密です。また、それは文化の問題、たとえば価値を溶解します 9.5 文字列になります "9,5" それ以外の "9.5" 一部の文化のために。

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

入力のケースを残したことに注意してください "11" の結果が得られます "10.5". 。それがバグかどうかはわかりませんが、それが元のコードが行うことです。

あなたの一般的な質問に、この特定の機能について他のレスポンダーが提案するようにリファクタリングできない他のケースについては、CCのバリアントが、ケースステートメントを単一のブランチとしてカウントするものがあります。理解を容易にするコード（テストカバレッジ用ではありませんが）。 1つのバリアントを測定する多くのツールが他のバリアントを提供します。代わりに、または使用しているものと同様に、ケース= 1バリアントを使用することをお勧めします。

ライセンス： CC-BY-SA と 帰属
所属していません StackOverflow
scroll top