Proper Practice when checking a List in a Constructor

Question

I have the following Book class (I've omitted the irrelevant code).

public final class Book {

    public Book(String bookTitle, List<Author> listofAuthors, int numberofchapters, String ISBN) 
    { 
      //Ommitted other variable initializations. 
      this.checkAuthors(listofAuthors);
      this.listofAuthors = Collections.unmodifiableList(new ArrayList<Author>(listofAuthors));          
    }

    private void checkAuthors(List<Author> checkifEmpty)
    {   
       if(checkifEmpty == null){
         throw new IllegalArgumentException( "List of authors is null");
       }

       if (checkifEmpty.size() == 0){
          throw new IllegalArgumentException( "The authors list contains no authors");
       }

       for (Author checkEmpty : checkifEmpty){
          if(checkEmpty == null){
            throw new IllegalArgumentException("A book must have at least one author");
        }
    }

}

I included a private method to check the List of authors to make sure that the List collection isn't 0, and to prevent

Author bookAuthor = null;

List<Author> listofAuthors = new ArrayList<Author>();
listofAuthors.add(bookAuthor);

or

List<Author> listofAuthors = null;
Book tlac = new Book("book title goes here", listofAuthors, 30, "isbn goes here");

from happening.

Questions:

Is calling a private method from the constructor and doing the work there considered a bad practice? If so, how should I correct it?
In the private method checkAuthors I use the == instead of equals(). My intention is to check that no object stored in the List<Author> has a value of null. Is this case, I found using == works, if I were to use equals() it give me null as the error message. Is using == correct in this case, if not, what should I be doing?

score 0 · Answer 1 · answered Mar 16 '17 at 18:25

Is calling a private method from the constructor and doing the work it's doing considered bad practice? If so, how should I correct it?

No, go for it as long as it makes sense, i.e. reduces the complexity of a single method or DRYs up the code.

In the private method checkAuthors I use the == instead of equals(). My intention is to check that no object stored in the List has a value of null. Is this case, I found using == works, if I were to use equals() it give me null as the error message. Is using == correct in this case, if not, what should I be doing?

Depends how specialised your object has to be. If this is crucial state-checking that is used in multiple places in your application then failing fast on construction is a good idea. Consider using something like Guava's preconditions to save yourself some typing.

Proper Practice when checking a List in a Constructor

1 Answers1