3

Still relatively new to Java and I'm wondering which is the better way to handle this. I have a class constructor that takes a few parameters, and also in this class are public getters and setters:

private String name;
private Float value;

public MySampleClass(String theName, Float theValue) {
    setName(theName);
    setValue(theValue);
}

public void setName(String n) {
    this.name = n;
}

public value setValue(Float v) {
    this.value = v;
}

I'd like to do some bounds checking on this Float. It seems like the best place to put it would be in the setter:

public value setValue(Float v) {
    if (v < 0.0f) {
        this.value = 0.0f;
    } else if (v > 1.0f) {
        this.value = 1.0f;
    }
}

This code originally had the bounds checking in the constructor and again in the setter, which seemed redundant. I changed the constructor to call the setter and put the checks in there. Does that make more sense? Or am I violating some convention of which I am completely unaware?

AWT
  • 3,657
  • 5
  • 32
  • 60
  • In my opinion you're doing it right. (A) point of the setter is to provide validation to "protect" your class's properties. – Sepster Sep 13 '12 at 16:01
  • 1
    Obviously in the setter because it's been exposed here and so the value can be potentially changed after the object is created. – Vikdor Sep 13 '12 at 16:02
  • Actually there is no correct way to do it, i don't like your way, i would let the setter just to set the value, and perform the bound checkings outside the setter, since it might be confusing in the future, or at least sepparate it in a private function in the class, like private value valueBoundsChecking(val){(logic....)return x} And then, setValue(value v){this.value=valueBoundsChecking(v);}, or something like that – F. Mayoral Sep 13 '12 at 16:03
  • I just want to point out that in the example posted, validation is being done both in the constructor and in the setter. The validation code itself is not within the constructor itself, but the validation is still run when `new MySampleClass()` is run - which is a slightly different question than the question might be interpreted as. What you've done here is just intelligent reuse of code. – matt b Sep 13 '12 at 16:04

2 Answers2

8

Calling overridable methods from your constructor is a bad idea. Do something more like this:

private String name;
private Float value;

public MySampleClass(String theName, Float theValue) {
    this.name = theName;
    setValueImpl(theValue);
}

public void setName(String n) {
    this.name = n;
}

public void setValue(Float v) {
    setValueImpl(v);
}

private void setValueImpl(Float v) {
    if (v < 0.0f) {
        this.value = 0.0f;
    } else if (v > 1.0f) {
        this.value = 1.0f;
    }
}

This gives you the validation in both places and eliminates the calls to overridable methods. See this question for more on this.

Edit: If you plan on subclassing MySampleClass and want the validation setter available, declare it protected final instead of private.

Community
  • 1
  • 1
Brian
  • 17,079
  • 6
  • 43
  • 66
  • 2
    +1 for this "Calling overridable methods from your constructor is a bad idea" – RNJ Sep 13 '12 at 16:13
  • Good points, I hadn't considered the possibility of overridable methods. Also, I plan to eliminate the needless internal call to setName. – AWT Sep 13 '12 at 16:28
2

For fairly simple data checks, such as your example, then yes, it makes the most sense to do the validation in the setter. However, if the validation for theValue also depends on theName (or on other things), then it would probably be worthwhile to perform the validation in the constructor (or on a private method that the constructor calls).

  • Better still to avoid calling setters internally if there is no real need to do so (calling setter for theName for instance). – Neil Sep 13 '12 at 16:04