Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have the following class :

public class Project {

    private int id;
    private String name;  

    public Project(int id, String name, Date creationDate, int fps, List<String> frames) {
        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) { 
        this.id = id;
    }

    public String getName() {
        return name;
    }

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

}

I have three questions:

  1. Do I use the class setters instead of setting the fields directly. One of the reason that I set it directly, is that in the code the setters are not final and they could be overridden.

  2. If the right way is to set it directly and I want to make sure that the name filed is not null always. Should I provide two checks, one in the constructor and one in the setter.

  3. I read in effective java that I should use NullPointerException for null parameters. Should I use IllegalArgumentException for other checks, like id in the example.

share|improve this question

2 Answers 2

Your class allows someone to say myProject.setName(null), avoiding the null check in the constructor. If you want to set stuff through the constructor but need checks (question 2), I suggest doing the checks in set-methods and call those from the constructor. Example:

public class Project {

    private int id;
    private String name;  

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

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

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

}

This avoids code duplication.

share|improve this answer
    
I thought about adding validation on setters only, after I read this answer I had some doubt if its the right way to do it: stackoverflow.com/a/4893604/302707. –  Jimmy Sep 1 '12 at 15:41
1  
@Jimmy then make a private method that checks that the variable is valid, but set the variable in the constructor. The set-method should also call that validator-method. This way you avoid duplication. –  Matsemann Sep 1 '12 at 15:53

I believe you shouldn't get anything extra in the getters and setters. There will be time in the future where putting extra functionalities in getters and setters cause weird side effects.

But the most compelling reason is when you use the class as data entity, those getters and setters might be used by java bean container (EJB, Spring), JPA, XML marshaller and unmarshaller.

I also prefer using IllegalArgumentException over NPE. If you plan to catch and recover or just log the error, it's almost always better to be as specific as possible.

But as you mentioned already, for validation. Having a separate validation method is much better.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.