CODEWITHSUNDEEP
javauser-inputsetter
Andrew the Programmer
Andrew the Programmer

Reputation: 269

More Elegant Way of Coding a Setter

I was wondering if their was a smarter and more elegant way of writing a setter, or for that matter, any code which has to check for whether user input is correct

This just seems odd. I'm a novice, so maybe this is just the standard way of approaching this problem, but if not, I'd appreciate knowing what the 'smart' way is

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

Upvotes: 1

Views: 142

Answers (4)

Krzysztof Cichocki
Krzysztof Cichocki

Reputation: 6414

Rules for writing "good" stter:

1) Don't do any Input-Output operations in setter like accessing DB, file, network, email sending, reading from command line etc... If you must, do it before or after you call setXXX, best writing some service method for that.

2) Have as small logic as possible in setter. It is ok to do parameter validation like range check etc...

3) Setter should do only the one thing it is designed for (set a value), it shouldn't have any side-effects.

Upvotes: 1

Kamal Kunjapur
Kamal Kunjapur

Reputation: 8840

The setters and getters are also known as mutators and accessors.

My understanding is that these methods are only there to retrieve or modify the 'state' of an object. It should not have the business logic or rather have as less business logic in it as possible.

Basically have Computations/Business Logic which are cheap.

Make your setter to only 'set' the values. 

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

While method to retrieve values based on input could be re-used anywhere else and that I suggest you can create a utility class and have that as method as static. 

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

This class should have the business logic to retrieve the details from user inputs and thereby should be used while trying to set the value of that bean instance.

Upvotes: 2

Endzeit
Endzeit

Reputation: 5474

First of all you can simplify your code like this:

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

    this.month = input;
 }

In addition to that I'd separate this into two functions. A setter should only set a value.

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

As suggested by ChiefTwoPencils, you can furthermore extract the setter into an additional class. Thus separating user input and data even more.

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

Upvotes: 4

blackdog
blackdog

Reputation: 2107

The function should be simple and only do one job. Maybe you can code like this: 

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

As you can see the function only do one thing that is set the month. If the argument isn't correct, throw the exception.

Upvotes: 1

Related Questions