2

次のメソッドのリファクタリングについてアドバイスをお願いします。

public boolean makeDecision(String group, int level, int primaryAmount, int secondaryAmount)
{
    if (group.equals("A"))
    {
        switch (level)
        {
            case 0 : return primaryAmount > 10000;break;
            case 1 : return primaryAmount > 20000;break;
            default : return secondaryAmount > 30000; break;
        }
    }
    else if (group.equals("B"))
    {
         switch (level)
         {
              case 0 : return primaryAmount > 40000;break;
              case 1 : return primaryAmount > 50000;break;
              default : return secondaryAmount > 60000; break;
         }

    }
    else if (group.equals("C"))
    {
        switch(level)
        {
            case 0 : return primaryAmount > 70000;break;
            case 1 : return primaryAmount > 80000;break;
            default : return secondaryAmount > 90000; break;
        }

    }
    return false;
}

私が達成したいこと：

  • 時間内により多くのグループ/レベルが存在するため、コードがオープン/クローズの原則に従うようにします。
  • 「レベル」スイッチ ステートメントの重複を削除します。
  • 理想的には、'group' 最上位の switch ステートメントを削除します。
4

2 に答える 2

1

各ケースは自明な比較によって計算されるため、すべての比較をどちらの方法でも行うことができます。

だからここに提案があります：

boolean[] aSol = { primary > 10000, primary > 20000, secondary > 30000 };
boolean[] bSol = { primary > 40000, primary > 50000, secondary > 60000 };
boolean[] cSol = { primary > 70000, primary > 80000, secondary > 90000 };

level = Math.min(level, 2);
return group.equals("A") ? aSol[level] :
       group.equals("B") ? bSol[level] :
       group.equals("C") ? cSol[level] :
       false;

かなり読みやすく、保守しやすいと思います。

これは、別のわずかに異なる定式化です。

boolean[][] result = {
        { primary > 10000, primary > 20000, secondary > 30000 },
        { primary > 40000, primary > 50000, secondary > 60000 },
        { primary > 70000, primary > 80000, secondary > 90000 } };

int groupId = Arrays.asList("A", "B", "C").indexOf(group);

if (groupId == -1)
    return false;

boolean[] groupResult = result[groupId];
return groupResult[Math.min(level, groupResult.length-1)];

別のオプションは、メソッドを使用してインターフェイスを作成することです

makeDecision(int level, int primaryAmount, int secondaryAmount)

次に、Map<String, GroupDecision>決定手順を入力します。

groupMap.put("A", new GroupDecision() { ... });
groupMap.put("B", new GroupDecision() { ... });
groupMap.put("C", new GroupDecision() { ... });

そして電話する

return groupMap.get(group).makeDecision(level, primaryAmount, secondaryAmount);

このアプローチは、おそらく最も拡張可能で読みやすいアプローチです。

于 2012-05-09T07:59:39.703 に答える
1

グループは、行動を持っているように見えます。文字列グループをファースト クラス タイプに昇格させることができます。クラスにメソッドを配置して、取得したロジックを表すこともできます。私は変数に馬鹿げた名前をつけました。

public class Group {
    public static Group A = new Group(10000,20000,30000);
    public static Group B = new Group(40000,50000,60000);
    public static Group C = new Group(70000,80000,90000);

    private int primaryMin;
    private int primaryMid;
    private int secondaryMax;

    private Group(int min, int mid, int max) {
        primaryMin = min;
        primaryMid = mid;
        secondaryMax = max;
    }

    public boolean getLevel(int level, int primaryAmount, int secondaryAmount) {
       if (level == 0)
         return primaryAmount > primaryMin;
       else if (level == 1) 
         return primaryAmount > primaryMid;
       else 
         return secondaryAmount > secondaryMax;
    }
}

したがって、トップレベルのステートメントを次のように減らすことができます

public boolean makeDecision(Group group, int level, int primaryAmount, int secondaryAmount) {
  return group.getLevel(level, primaryAmount, secondaryAmount);
}

null オブジェクト パターンを使用して不明なグループを処理することを検討することをお勧めします。

Levelレベルが時間とともに成長すると言う場合は、クラスを導入し、ポリモープシムの別のレイヤーとして if/else チェーンをそこにプッシュするために、ほぼ同じことをもう一度行うことを検討します。これは、最初に の型でディスパッチし、次にの型でディスパッチする二重ディスパッチ パターンになります。これは、既存のコードを変更する必要なく、新しいコードを追加できることを意味します。GroupLevel

于 2012-05-09T08:19:13.130 に答える