Is validating fields in both constructor and setter considered bad redundant code?

Question

I have the following class :

public class Project {

    private int id;
    private String name;  

    public Project(int id, String name) {
        if(name == null ){
            throw new NullPointerException("Name can't be null");
        }

        if(id == 0 ){
            throw new IllegalArgumentException("id can't be zero");
        }

            this.name = name;
            this.id = id;

    }

    private Project(){}

    public int getId() {
        return id;
    }

    public void setId(int id) { 
        if(id == 0 ){
            throw new IllegalArgumentException("id can't be zero");
        }
        this.id = id;
    }

    public String getName()
        return name;
    }

    public void setName(String name) {
        if(name == null ){
            throw new NullPointerException("Name can't be null");
        }
        this.name = name;
    }

}

If you noticed that setName and setId share the same validation for its fields with the constructor. Is this redundant code that could cause issues in the future ( for example if somebody edit the the setter to allow 0 for the id and prevent -1 instead but didn't change the constructor) ? . Should I use a private method to do the check and share it between the constructor and the setter which seems too much if there's a lot of fields.

Note: This is why im not using the setters in the constructor. https://stackoverflow.com/a/4893604/302707

Or (in case a setter cannot be called from Constructor), pass the validation logic to a shared, private method. — SJuan76, Sep 01 '12 at 16:31
what's with the `private` constructor and validation in the getter? — mre, Sep 01 '12 at 16:32
@Captain Giraffe: This is why im not using the setter http://stackoverflow.com/a/4893604/302707 — Jimmy, Sep 01 '12 at 16:33
This has nothing to do with the question but you should use `if(id <= 0)` instead of `if(id == 0)`. i can see no point of having id as a negative value if the 0 is not valid unless there is a special reason to it. — gigadot, Sep 01 '12 at 16:38
@Captain Giraffe: So what if someone override the setter without adding the validation (setters are not final) then projects subclass objects will not be valid. — Jimmy, Sep 01 '12 at 16:39
@Jimmy With overriding comes responsibility. Take the calls to super in a large amount of the Android API for instance. — Captain Giraffe, Sep 01 '12 at 16:40
Can you provide an example (maybe from the Android source code?), I'm an Android app developer myself and i'd love to be proven wrong. — Jimmy, Sep 01 '12 at 16:43
@Jimmy, if you are an Android developer you are plenty aware of all the super calls in an Activity for instance. — Captain Giraffe, Sep 01 '12 at 17:32
How is this relevant, the Activity has only one default constructor (they dont use setter their). — Jimmy, Sep 01 '12 at 18:11

score 2 · Answer 1 · answered Sep 01 '12 at 16:42

Here is the revised code:

public class Project {

    private int id;
    private String name;  

    public Project(int id, String name, Date creationDate, int fps, List<String> frames) {

        checkId(id);
            checkName(name);
            //Insted of lines above you can call setters too.

            this.name = name;
            this.id = id;

    }

    private Project(){}

    public int getId() {
        return id;
    }

    public void setId(int id) { 
        checkId(id);
        this.id = id;
    }

    public String getName()
        return name;
    }

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

    private void checkId(int id){
       if(id == 0 ){
            throw new IllegalArgumentException("id can't be zero");
        }
    }

    private void checkName(String name){
            if(name == null ){
            throw new NullPointerException("Name can't be null");
        }
    }

}

this is a trade off and depends on the developer requirement. depends on the way that they maintain their code. — Mehdi, Sep 01 '12 at 16:45

Bharat Sinha · Answer 2 · 2012-09-02T02:43:17.400

1

I recommend that you should define one method per field as isValid() and then call the same method in you setter as well as Constructor.

edited Sep 02 '12 at 02:43

answered Sep 01 '12 at 16:30

Bharat Sinha

13,973
6
39
63

But how do you think isValid could work for both id and name. – Jimmy Sep 01 '12 at 19:02

score 0 · Answer 3 · answered Sep 01 '12 at 16:34

0

I would say yes. Rather than that, just call the setters from your constructor:

public Project(int id, String name, Date creationDate, int fps, List<String> frames) {
    setName(name);
    setId(id);
    // other stuff with creationDate, fps and frames?
}

Also, you shouldn't check for a null name in getName -- do it in setName. Otherwise, bugs are going to be hard to track down -- you want to catch the invalid name as soon as it comes in, not when it's used (which may be much later).

answered Sep 01 '12 at 16:34

yshavit

42,327
7
87
124

IMHO, this is bad advice...unless you made the mutator methods `final`. – mre Sep 01 '12 at 16:40
bad because if someone overrides `setName` and `setId` it could be catastrophe. it is not recommended to call non-private or non-final method in the constructor within the same class – gigadot Sep 01 '12 at 16:46
Good catch to both; you should make the mutator methods (or the whole class) `final` if you go with this approach. Of course, if you go with an approach like `isValid(String)` or `checkName(String)`, the same condition applies. – yshavit Sep 01 '12 at 23:12

score 0 · Answer 4 · edited May 23 '17 at 11:59

0

if you make Project immutable, it will eliminate the redundant code. but for now, i think explicitly throwing exceptions in both the constructor and mutator methods is fine.

and i would not call the mutator methods within the constructor for many reasons, including this. also, i would remove the validation code in the accessor method...it's not necessary.

edited May 23 '17 at 11:59

Community

1
1

answered Sep 01 '12 at 16:35

mre

43,520
33
120
170

Is validating fields in both constructor and setter considered bad redundant code?

4 Answers4