CODEWITHSUNDEEP
javaconstructormutators
pgmank
pgmank

Reputation: 5819

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

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 ?

Upvotes: 2

Views: 2277

Answers (3)

Marco13
Marco13

Reputation: 54611

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).

Upvotes: 4

Akzwitch
Akzwitch

Reputation: 113

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.

Upvotes: 0

Rohit Jain
Rohit Jain

Reputation: 213203

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

Upvotes: 0

Related Questions