1

I always used setters as a place to validate a value before assigning it to a field. 

public class Person {
    private int id;

    public int getId() {
        return this.id;
    }

    public int setId(int id) {
        if (id < 0) {
            throw new IllegalArgumentException("id must be positive.");
        }
        this.id = id;
    }

    public Person(int id) {
        setId(id);
    }
}

What I think is good about it is that I now have one place where a value gets validated before assignment and I can call it from everywhere. But the compiler warns me about that overridable method call in the constructor of the class. I read What's wrong with overridable method calls in constructors? and understand the issues this might cause and that it is recommended to use a Builder Pattern instead. But would it be also okay to just add final to the getters and setters instead to prevent overriding those properties?

Community
  • 1
  • 1
urbaindepuce
  • 503
  • 1
  • 5
  • 17
  • You could of course, but in many situations it gives cleaner code to separate validation from data objects. – Johan Sjöberg Dec 20 '14 at 10:50
  • I think making the setter final is a good solution. In my humble opinion a better solution would be to get rid of the setter and make your class immutable. – Paul Boddington Dec 20 '14 at 11:02
  • @pbabcdefp You mean by making the validation a private method as suggested in the answer? – urbaindepuce Dec 20 '14 at 11:22
  • No I mean putting the validation in the constructor and getting rid of the method setId completely. Instead of setting the id on an existing instance you'd have to create a new instance. IMHO setX methods are massively overrated. – Paul Boddington Dec 20 '14 at 11:27

1 Answers1

0

would it be also okay to just add final to the getters and setters instead to prevent overriding those methods?

Yes, it would. In addition to stopping the warning, it would make your code more robust, because subclasses would not be able to change the validation in your setters, either on purpose or by accident.

Another solution would be to make a private method for validating the data, and call it from both the constructor and the setter. This would solve the problem as well:

private void setIdChecked(int id) {
    if (id < 0) {
        throw new IllegalArgumentException("id must be a positive.");
    }
    this.id = id;
}
public void setId(int id) {
    setIdChecked(id);
}
public Person(int id) {
    setIdChecked(id);
}
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • Thank you. I think that this is much cleaner and shorter than implementing the builder pattern, especially when working with smaller classes. In my case the object just holds data for a table row. – urbaindepuce Dec 20 '14 at 11:18