Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

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

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

like image 712
Jimmy Avatar asked Sep 01 '12 16:09

Jimmy


1 Answers

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");
        }
    }

}
like image 180
Mehdi Avatar answered Sep 20 '22 13:09

Mehdi