Is it a good practice to use set methods in the constructor to initialize class's fields?

Question

I would like to use a class's set methods in the constructor in order to check the values to be initialized, throwing an exception if they do not comply with the constraints I set.

code example:

public class MyClass {

    // Fields
    private int number;
    private String string;

    // Constructor
    public MyClass(int number, String string) {
        setNumber(number);
        setString(string);
    }

    // Set Methods
    public void setNumber(int number) {
        if (number<=0) {    // Certain constrain for number
            throw new IllegalArgumentException("Number must be positive");
        }
        this.number = number;
    }

    public void setString(String string) { // Certain constrain for string
        if (string.equals("")) {
            throw new IllegalArgumentException("string cannot be empty");
        } 
        this.string = string;
    }

    public String toString() {
        return String.format("Ordered %d x %s%n", number, string);
    }

    public static void main(String[] args) {
        MyClass obj = new MyClass(8, "Souvlaki");   // Everything allright
        System.out.println(obj);
        try {
            MyClass obj2 = new MyClass(-3, "Mousaka");  // Error in number argument
        } catch (IllegalArgumentException exception) {  // catch the exception
            System.out.printf("Exception Caught: Number must be positive%n%n");
        }
        MyClass obj2 = new MyClass(4, "");  // Error in string argument
        // Allow the exception to end program execution
    }
}

Output:

Ordered 8 x Souvlaki

Exception Caught: Number must be positive

Exception in thread "main" java.lang.IllegalArgumentException: string cannot be empty at MyClass.setString(MyClass.java:23) at MyClass.(MyClass.java:10) at MyClass.main(MyClass.java:40)

The output is just what I wanted. First object created is initialized with appropriate values. Calling toString() method implicitly proves that. The second and third objects throw exception due to wrong initialization. The first exception is caught in order to allow the program to continue executing. The second exception is not caught in order to output the error message printed be the exception.

So everything seem to be right,but is this a good programing technique or it hides some bugs inside ?

`if (string == "")` not again... http://stackoverflow.com/questions/513832/how-do-i-compare-strings-in-java — Pshemo, Mar 15 '15 at 09:40
[No, It's really a bad idea](http://stackoverflow.com/q/18138397/1679863). Please avoid doing that. — Rohit Jain, Mar 15 '15 at 09:41
Yes and no. Issues may arise if the method you are using assumes a certain state which can't be guaranteed until AFTER the class is constructed. Personally, I tend to lean towards this to ensure that inheritance can work, allowing sub classes to override the set methods as required and not need to resort to nasty hacks to make it work, but that's me... — MadProgrammer, Mar 15 '15 at 09:42
@RohitJain: If all the setters are made final (or if the class itself is final), that issue goes away. — Jon Skeet, Mar 15 '15 at 09:46
I would rather suggest to have a single method to validate invariants of objects of that class. And invoke it at the beginning of other methods using your class instance. — Rohit Jain, Mar 15 '15 at 09:53
@Pshemo , that's too silly of me. I have just corrected that. — pgmank, Mar 15 '15 at 14:42

score 4 · Accepted Answer · edited May 23 '17 at 10:28

As the comments suggest, there may be an issue with that. Particularly, you may want to have a look at What's wrong with overridable method calls in constructors?. The bottom line is roughly: Someone might override the set... methods in an unexpected way, and refer to other (uninitialized) fields of the class, which can cause all sorts of bugs.

Dedicated validation methods may be an option. But these may be called multiple times, even when there is no validation necessary.

You can alleviate most of the problems by making the set... methods final. This is a good practice anyhow. As Joshua Bloch says in his book "Effective Java", item 17:

"Design and document for inheritance or else prohibit it"

This means that you should make every method final, unless you explicitly want to allow it to be overridden, and document how it should be overridden (or, alternatively, make the whole class final).

Yes. AKA "letting `this` *escape* from the constructor" – Bohemian Mar 15 '15 at 15:01 — Bohemian, Mar 15 '15 at 15:01

score 0 · Answer 2 · answered Mar 15 '15 at 09:58

Rather than doing validation in the constructor, you can create a checkInvariant() method in your class, which validates all the fields.

class MyClass {
    private int num;
    private String value;

    public void checkInvariants() {
        assertNotEmpty(value, "String value cannot be empty");
        assertPositive(num, "Number num should be non-negative");
    }
}

And then somewhere else, where you might pass an instance of this class as argument, invoke this method first to ensure invariants hold:

class SomeOtherClass {
    public void doSomethingWithMyClass(MyClass myClass) {
        myClass.checkInvariants();
        // Proceed with work.
    }
}

score 0 · Answer 3 · answered Mar 15 '15 at 10:45

0

Your variables are accessible anywhere inside the class, so no need to use mutator methods to initialize your variables.

If you want to do some validation with the input parameters, use another method which performs all the validation required.

Call the validation method in your constructor.

answered Mar 15 '15 at 10:45

Akzwitch

113
1
2
13

If I did it this way, it would require to call twice this method. One time for the setter and one time for the constructor. Apart from adding one more method to the class, this method still needs to be **final** as @Marco13 mentioned in his answer to prevent unexpected errors like in the constructor. – pgmank Mar 15 '15 at 14:53

Is it a good practice to use set methods in the constructor to initialize class's fields?

3 Answers3