5

Many years ago, I posted a problem I had with some code and received a well written detailed answer that suggested the use of factory methods. I liked this approach, because I can provide a method name to let clients know what was being created, and to prevent wrong parameters.

// Left out additonal parameters and validation
private Student(StudentType type, List<String> documents) {
    this.type = type;
    this.documents = documents;
}

public static Student createDomestic() {
    return new Student(StudentType.DOMESTIC), Collections.emptyList());
}

public static Student createInternational(List<Document> documents) {
    return new Student(StudentType.INTERNATIONAL, new ArrayList<>(documents);
}

Benefits:

  • I don't need a private method to validate the List based on the StudentType, it made the code shorter and cleaner. All I needed was to check if the List provided for the International students contained documents (by checking the length of the List).
  • The static factory prevents clients from assigning a Domestic student a List of documents, which according to the requirements it shouldn't have.

Leaving aside the POLA, is using a static factory method the way it was suggested on SO, considered a code smell or bad practice?

6

This seems like a perfectly good use of static factory methods.

The requirement that a international student should have documents, but a domestic student shouldn't, does make me question why you didn't use an object hierarchy in which you have DomesticStudent and InternationalStudent classes deriving from Student, but I'll assume that there were other requirements driving that decision.

0

An simpler alternative might be to have a single constructor taking a single parameter, the List. Skeletal code:

public Student(List docs) {
  this.documents = docs;
  this.type = (docs.length() > 0)?
                         StudentType.INTERNATIONAL :
                         StudentType.DOMESTIC;
}

Not sure if this makes more sense for your application.

Also, makes you wonder if the type field is required - couldn't you just check for docs.length? e.g.

public boolean isInternational() {
  return docs.length() > 0;
}
  • 6
    I would advise against this approach. Say there was a bug in the calling code which meant that it attempted to create an international student but failed to supply documents. This code would just happily create a domestic student instead. Likewise, erroneously adding documents to a domestic student would result in the creation of an international student. – Pete Jun 8 '18 at 8:33
  • @Pete valid point, if you wish to require that the caller know whether the student is domestic / international. Or do they simply know if they have documents? My proposal encapsulates the domestic / international logic. – user949300 Jun 8 '18 at 15:23
  • @user949300 - you could keep your isInternational() method but rewrite the code to check the enum rather than the List – user306112 Jun 8 '18 at 16:02
  • @user949300 - This might go beyond the scope of the question, but I don't think that the Student should determine if it's Domestic or International. The University object would decide. In reality, while the student will know if he or she is domestic or international, the university will gather the documents and confirm. Simply telling the university official you're domestic or international won't mean much, why should they take your word over gathering the documents and checking? – user306112 Jun 9 '18 at 16:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy