Проверка полей в конструкторе и установщике считается плохим избыточным кодом?

Question

У меня есть следующий класс:

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

}

Если вы заметили, что setName и setId совместно используют одну и ту же проверку для своих полей с конструктором. Является ли это избыточным кодом, который может вызвать проблемы в будущем (например, если кто-то отредактирует установщик, чтобы разрешить 0 для идентификатора и запретить -1 вместо, но не изменил конструктор)?, Должен ли я использовать закрытый метод для проверки и делиться им между конструктором и установщиком, что кажется слишком большим, если полей много.

Примечание. Вот почему я не использую сеттеры в конструкторе. /questions/15599664/vyizov-settera-iz-konstruktora/15599675#15599675

Answer 1

Вот пересмотренный код:

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

}

Answer 2

Я рекомендую вам определить один метод для каждого поля как isValid() и затем вызовите тот же метод в вашем сеттере, как и в конструкторе.

Answer 3

Если вы сделаете Project неизменный, это устранит избыточный код. но сейчас я думаю, что явно выбрасывать исключения как в конструкторе, так и в методе мутатора - это нормально.

и я бы не стал вызывать методы-мутаторы в конструкторе по многим причинам, включая это. Кроме того, я бы удалил код проверки в методе доступа... это не обязательно.

Answer 4

Я бы сказал, да. Вместо этого просто вызовите сеттеры из вашего конструктора:

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

Кроме того, вы не должны проверять на ноль name в getName - сделай это в setName, В противном случае будет сложно отследить ошибки - вы захотите поймать недопустимое имя, как только оно появится, а не когда оно будет использовано (что может быть гораздо позже).