2016-05-16 19 views
-1

私は完全に動作する以下のコードを持っています。しかし、それを見るだけで、より短くてエレガントな方法が必要であると私は信じています。 Switch()は明らかに答えではないので、私は入れ子になっています。ネストされたIFを短くしてください

if (mode == 1) 
{ 
    if (distance <= 4000) 
    { 
     modeValue = "1F"; 
    } 

    else if (distance > 4000 && distance <= 8000) 
    { 
     modeValue = "2F"; 
    } 

    else if (distance > 8000 && distance <= 12000) 
    { 
     modeValue = "3F"; 
    } 

    else if (distance > 12000) 
    { 
     modeValue = "F 0-5"; 
    } 
} 

else if (mode == 2) 
{ 
    if (distance <= 500) 
    { 
     modeValue = ""; 
    } 

    else if (distance > 500 && distance <= 4000) 
    { 
     modeValue = "2F"; 
    } 

    else if (distance > 4000 && distance <= 8000) 
    { 
     modeValue = "3F"; 
    } 

    else if (distance > 8000 && distance <= 12000) 
    { 
     modeValue = "4F"; 
    } 

    else if (distance > 12000) 
    { 
     modeValue = "F 0-5"; 
    } 
} 

+2

[CodeReview](http://codereview.stackexchange.com/)でこの質問を試してください。 – Jonesopolis

+0

クリーンアップする方法の1つは、すべてを方法化することです。物事をするもう一つの方法は、上記のロジックに基づいてmodeValueを導出するメソッドを含むオブジェクトのプロパティを距離にすることです。それを完全に取り除くのではなく、もっと組織化しています。 –

+0

短い方が良いとは限りません。コードが正常に機能している場合は、理解して維持しやすいようにしてください。 – Nasreddine

答えて

1

まず第一に、あなたはスイッチでmodeに対処if文を置き換える必要があります。

switch (mode) { 
case 1: 
    //... 
    break; 
case 2: 
    //... 
    break; 
default: break; 
} 

次に、あなたが方法でそれらを配置することによって、スイッチと、内側のif-elseチェーンを交換することができます(ありますこれらはC#の「手順」と呼ばれています):

string Foo(int mode, int distance) { 
    switch (mode) 
    { // http://stackoverflow.com/users/469563/gendoikari Thanks for pointing out redundant comparisons 
    case 1: 
     if (distance <= 4000) 
     { return "1F"; } 
     else if (distance <= 8000) 
     { return "2F"; } 
     else if (distance <= 12000) 
     { return "3F"; } 
     else 
     { return "F 0-5"; } 
    case 2: 
     if (distance <= 500) 
     { return ""; } 
     else if (distance <= 4000) 
     { return "2F"; } 
     else if (distance <= 8000) 
     { return "3F"; } 
     else if (distance <= 12000) 
     { return "4F"; } 
     else 
     { return "F 0-5"; } 
    default: break; 
    } 

ああ、すごくうれしいことが起こった! modeValueを変更したい場合は、今すぐFooに電話することができます。

modeValue = Foo(mode, distance); 
+0

これはちょうど私が探していたすっきりしたソリューションの種類です!ありがとう、dorukayhan! – PeterJ

+0

それは実際にはコンパイルされません! (返品時にデフォルトのケースが必要) – Chris

+0

ああ...警告ありがとう – dorukayhan

2

カップルの事:

まず第一に、あなたが持っている冗長チェック

else if (distance > 500 && distance <= 4000)

は、単にので、「他」の部分で、代わりにelse if (distance <= 4000)可能性が..あなたはすでにその距離があるチェックしていますあなたが他にいるならば> 500。

第2に、本当にこれを取り巻く文脈に依存しますが、これは複数のクラスを使用するのに適しています。ここではCalculateModeValueというメソッドの内部にあると仮定し、そのメソッドはCalculatorというクラスの内部にあると仮定します。また、 "Mode == 1"は "LongDistance"を意味し、 "Mode == 2"は "ShortDistance"を意味すると仮定します。

この場合、Calculatorという抽象クラスがあります。私は2つの別々のクラス、 "LongDistanceCalculator"と "ShortDistanceCalculator"でこれをサブクラス化します。 CalculateModeValueはCalculatorの抽象メソッドであり、各サブクラスで個別に実装されます。この方法では、 "If Mode == 1"を確認する必要はありません。各クラスの実装は、そのモードの正しいロジックを処理します。

ここでも、これは未知数についての仮定をしています。実際には、これらのif文の前後のコンテキストに依存します。

+0

*本当にこれらのif文の前後のコンテキストに依存します。 –

1

ネストされたifsを使用する方法は間違いありません。 しかし、あなたが本当にそれらを見たくない場合は、単純に、モード変数を取り込み、その後、モードの値にロジックを適用する方法でロジックを入力するだけですか?

このようなサブプロシージャを作成すると、アプリケーションで同じ計算を適用するために再利用できます。

0

すべての最初さて、私は括弧なしの遺体ワンライナーならば、それは本当の良いそれを短くします作ると思います。あなたはそれを好きではない場合は、それを行う方法のトンがあります

string Func1(int mode, double distance) 
{ 
    if(mode == 1) 
    { 
     if(distance <= 4000) 
      return "1F"; 
     else if(distance <= 8000) 
      return "2F"; 
     else if(distance <= 12000) 
      return "3F"; 
     else 
      return "F 0-5"; 
    } 
    else 
    { 
     if(distance <= 500) 
      return ""; 
     else if(distance <= 4000) 
      return "2F"; 
     else if(distance <= 8000) 
      return "3F"; 
     else if(distance <= 12000) 
      return "4F"; 
     else 
      return "F 0-5"; 
    } 
} 

:いくつかのマイナーな改良とあなたの元のコードは、それが私見できると同じくらい良いになります。方法:

class Mode 
{ 
    public double Distance; 
    public string Name; 
    public Mode(double d, string n) { this.Distance = d; this.Name = n; } 
} 

string Func(int mode, double distance) 
{ 
    Mode[] modes; 
    if(mode == 1) 
     modes = new Mode[] { new Mode(4000, "1F"), 
          new Mode(8000, "2F"), 
          new Mode(12000, "3F"), 
          new Mode(double.MaxValue, "F 0-5") }; 
    else 
     modes = new Mode[] { new Mode(500, ""), 
          new Mode(4000, "2F"), 
          new Mode(8000, "3F"), 
          new Mode(12000, "4F"), 
          new Mode(double.MaxValue, "F 0-5") }; 

    for(int pos = 0; ; pos++) 
     if(distance <= modes[pos].Distance) 
      return modes[pos].Name; 
} 

明らかに、これらの配列を再利用できます。または、各モードにPredicate<T>などのモードを指定して、そのモードを選択する条件を指定することもできます。そして、私はModeクラスを単純にKeyValuePair<TKey,TValue>にして、もう少し短くすることができると思います。

関連する問題