5

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

6 に答える 6

15

なぜ持っていないの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、キーが重複している場合は、型を初期化するときに例外が発生するため、エラーをすぐに見つけることができます。

このように言えば、辞書ベースのバージョンから「たくさんの実際のコード」バージョンへのリファクタリングを検討したことがありますか？私は確かにそうしません。

本当に、本当にすべてをメソッドに入れたい場合は、いつでも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;
    }
}

私はまだ自分で辞書形式を使用します...しかし、これには、コンパイル時に重複検出が行われるという非常にわずかな利点があります。

于 2011-10-18T22:18:29.070 に答える
3

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

  • 「FourOrMore」を5に変換しますが、「MoreThanTen」は10.5に変換されます。これは一貫性がないようです。
  • 「11」を10.5に変換しますが、これも他のコードと矛盾しているようです。

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

于 2011-10-18T22:26:16.403 に答える
0

うん。確かに非常にメンテナンス可能です。

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

// 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は非常に高くなります。適切なデータ構造を使用して問題を解決することで、コードを整理して再利用できるようになります。

于 2011-10-18T22:19:57.180 に答える
0

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

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

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

于 2011-10-18T22:29:20.253 に答える
0

DRYの原則（自分自身を繰り返さないでください）を使用すると、これらすべての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"。それがバグかどうかはわかりませんが、元のコードはそれを実行します。

于 2011-10-18T22:35:58.403 に答える
0

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

于 2011-10-20T08:26:59.040 に答える