2016-10-16 6 views
1
彼らのセッターを書いて賢く、よりエレガントな方法だった、またはそのことについては、どうか、ユーザー入力をチェックするために持っている任意のコードが

もっとエレガントな方法

これを単に正しいかどうか、私は思っていた

奇妙に思える。私は多分、これはちょうどこの問題にアプローチするための標準的な方法で、初心者だけど、いない場合は、私が「スマート」な方法は、あなたのコードを簡素化することができ、すべての

public void setMonth(){ 
    boolean run = true; 
    while(run){ 
     int input = scan.nextInt(); 
     if(input > 0 && input < 13){ 
      this.month = input; 
      run = false; 
     } 
    } 
} 
+4

提案:セッターにユーザー入力をしないでください。入力をセッターに渡すだけで、入力を検証するだけです。メソッドの外部への入力を促します。 – Li357

+2

このメソッドを3つのステップに分割する必要があります。1. getInput()2. validate()3. setValue()。現在このメソッドは3つの責任を果たしていますが、これは好ましくありません。 – techtrainer

+2

あなたが表示する方法は、一般的にJavaコンテキストでは「セッター」と呼ばれるものではありません。 – hyde

答えて

0

ルール:あなたなら...電子メールは、コマンドラインなどからの読み取り、送信、DB、ファイル、ネットワークにアクセスするようにセッターに任意の入出力操作をしないでください

1)

setXXXを呼び出す前または後に、そのためのいくつかのサービスメソッドを書くのが最善です。

2)できるだけ小さなロジックをセッターで使用してください。範囲チェックなどのパラメータ検証を行うのは問題ありません。

3)セッターは、設計された1つのもの(値を設定)のみを実行する必要があります。副作用はありません。

4

まず何であるかを知っていただければと思います

public void setMonth() { 
    int input = 0; 
    do { 
     input = scan.nextInt(); 
    } while(input <= 0 || input >= 13) 

    this.month = input; 
} 

これに加えて、これを2つの機能に分けます。設定者は値を設定するだけです。

public void setMonth(int month) { 
    if(month > 0 && month <= 12) { 
     this.month = month; 
    } else { 
     throw new IllegalArgumentException("Month has to be a value between 1 and 12 inclusively. Actual value was :" + month); 
    } 
} 

public void readMonth() { 
    int input = 0; 
    do { 
     input = scan.nextInt(); 
    } while(input <= 0 || input >= 13) 

    setMonth(input); 
} 

ChiefTwoPencilsが示唆しているように、さらにセッターを追加クラスに抽出することができます。したがって、ユーザの入力とデータをさらに分離することができます。

class MonthDataHolder { 
    private int month; 

     public MonthDataHolder(int month) { 
      setMonth(month); 
    } 

    public void setMonth(int month) { 
     if(isValidMonthValue(month)) { 
      this.month = month; 
     } else { 
      throw new IllegalArgumentException("Month has to be a value between 1 and 12 inclusively. Actual value was :" + month); 
     } 
    } 

     private boolean isValidMonthValue(month) { 
      return month >= 1 || month <= 12; 
     } 
} 

class InputReader { 

     public MonthDataHolder readMonth() { 
     int input = 0; 
     do { 
      input = scan.nextInt(); 
      } while(input <= 0 || input >= 13) 

      return new MonthDataHolder(input); 
     } 
} 
1

セッターとゲッターは、ミューテーターとアクセサーとも呼ばれます。

私の理解では、これらのメソッドは、オブジェクトの '状態'を取得または変更するためのものであると私は理解しています。ビジネスロジックを持つべきではなく、できるだけビジネスロジックを持たないようにするべきです。

基本的に安い計算/ビジネスロジックがあります。

セッターに値を「設定」するようにしてください。

public void setMonth(int month){ 
    this.month = month 
} 

の入力に基づいて値を取得するメソッドが他の場所で再利用し、私はあなたがユーティリティクラスを作成し、静的の方法としてあることを持つことができることを示唆することができますが。

class DateUtil { 
    public static int retrieveMonthBasedOnInput(){ 
    } 
} 

このクラスは、ユーザ入力から詳細情報を取得するために、ビジネスロジックを持っている必要があり、それによってそのBeanインスタンスの値を設定しようとしているときに使用する必要があります。

+1

あなたは 'setMonth()'でセッターをしません。代わりに、副作用を伴う何らかの方法で、混乱しています。それは良い習慣ではありません。 –

+0

このクラスは 'DateUtil'に依存しています。確かに月をパラメータとして渡し、外部からプロンプトを出すことは適切なデザインです –

+0

@KrzysztofCichockiそれが見つかりました!私はUtilクラスを持つ方がおそらく良い方法だと思います。 – Kamal

0

この機能はシンプルで、1つのジョブだけを実行する必要があります。多分あなたは次のようにコード化することができます:

public void setMonth(int month) throws IllegalArgumentException { 
    if(month<0 || month>11) throw new IllegalArgumentException("Month can only be set 0~11"); 
    this.month = month; 
} 

ご覧のとおり、月に設定されているものは1つだけです。引数が正しくない場合は、例外をスローします。書き込み「良い」stterため

関連する問題