1
\$\begingroup\$

For a project I'm working on I have the following requirements:

We have two student types: Domestic and International

public enum StudentType {
    Domestic,International
}

Students of both types must have a ID number, First/Last name but only international students require documentation (Passports, drivers licence, etc).

I've decided to create the Student class as immutable, and came up with the following implementation:

public final class Student {

    private final String id;
    private final String firstname;
    private final String lastname;
    private final Collection<String> documents;;
    private final StudentType typeofStudent;

    private Student(String id, String firstname, String lastname, List<String> documents, StudentType typeofStudent)
    {   
        this.id = id;
        this.firstname = firstname;
        this.lastname = lastname;
        this.documents = Collections.unmodifiableList(new ArrayList<String>(documents));
        this.typeofStudent = typeofStudent;
    }

    public static Student createDomestic(String id, String firstname, String lastname)
    {
        return new Student(id, firstname, lastname, Collections.emptyList(), StudentType.Domestic);
    }

    public static Student createInternational(String id, String firstname, String lastname, List<String> documents) 
    {   
        return new Student(id, firstname, lastname, documents, StudentType.International);
    }
} 

I left out constructor validation to keep my code short and concise.

Advantages of the static method approach:

  • If the constructor was public, I would need to write a private helper method that checks if the student is domestic, the client didn't pass a list of documents. I would throw an exception to notify the client if this happened.
  • My code is shorter because I prohibit the client from passing a List<String> documents if the student was domestic. I simply ask for the ID, and first/last name. The Collections.unmodifiableList(); and StudentType are set when domestic students are created. In other words, no need for a private helper method.
  • The client knows exactly what type of student is being created without calling a getter/internal method to verify the student type.

Problem:

I came across this SESE page that was talking about the disadvantages of static. I became confused and wanted someone to review my code and tell me if my use of static methods are okay given what was said on that page?

The code and answer on this page provide what I need.

\$\endgroup\$
  • \$\begingroup\$ Maybe I don't understand your problem correctly, but to me this seems like a typical application of inheritance. Why not have a base class Student and two child classes called InternationalStudent and DomesticStudent? \$\endgroup\$ – Ben Steffan Oct 26 '17 at 18:43
  • \$\begingroup\$ @BenSteffan - I could but I want to keep it immutable. Let me clean up my question and see if that helps to clarify things. \$\endgroup\$ – user150904 Oct 26 '17 at 18:56
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success Oct 27 '17 at 18:25
3
\$\begingroup\$

There are many code smells:

  • You create a single Student class, and differentiate students by an explicit type. This is a bit sad in an object-oriented language, where one should rather attempt to use interfaces and classes to represent types, leveraging the power of abstraction and polymorphism. Typically, your design would not scale well if you were to add new student types, or specific student behaviors (SRP / OCP from SOLID are at risk).
  • You create a property which should not be set not read (documents). This code is therefore a bit dubious (not useful, prone to misinterpretation).

Back to your domain: a student necessarily originates from somewhere (domestic), and may study abroad (international). In other words, I think you should simply have InternationalStudent inherit DomesticStudent, extending the class with the documents.

As for immutability, you don't have many options. The members must be final, and thus must be initialized during construction, with arguments passed to the constructor (either separately, or grouped inside a value object). Collections returned through getters must be wrapped by unmodifiable. You are already aware of this.

As for the creation method, you don't provide enough of your requirements to elicit a particular solution. Constructor calls or factories both seem fine to me.

Static methods may or may not be good, depending on the context. They do incite to consider a class as a namespace rather than a class (an object-orientation failure), and complicate testing (difficulty to mock dependencies).

\$\endgroup\$
  • \$\begingroup\$ Your comment about documents doesn't make sense, "You create a property which should not be set not read (documents)." Not set and not read. BTW both are wrong. \$\endgroup\$ – user150904 Oct 26 '17 at 22:25
  • \$\begingroup\$ Hi. It is an immutable empty list in one case, hence the comment. I could of course be wrong, as I don't know about your use cases (use cases drive the design). My point is to leverage object orientation, relocating business logic from non-student classes ("if student is of type X, do this"), to the two student classes, as additional methods. I suspect that after this refactoring, the documents property would become unnecessary on the DomesticStudent class. \$\endgroup\$ – Elegie Oct 27 '17 at 5:34
  • \$\begingroup\$ It'a already unnecessary, that's why I pass an empty list to it upon domestic student construction. If I were to leave it for international students only, I would have to downcast which could be messy. Effective java states rather than return null, create or return Collections.Empty(). However, I agree with the rest of your advice, it should be sub classed, or at least the status of the student should be subclassed, maybe a StudentStatus(abstract class) and Domestic/International, while the Student class remains immutable. \$\endgroup\$ – user150904 Oct 27 '17 at 12:40
1
\$\begingroup\$

if the constructor were made public I would then have to validate the student, using a private method, to ensure that if the student is domestic, the client can't pass a list of documents by throwing an exception. The static methods allowed the creation of the student objects and the client can't pass a List documents for domestic students, and there is no need to throw an exception.

This assumption is simply wrong because your factory method does not check any argument. So there is no difference whether you create your Student objects vis your factory method or not.

On the other hand having a factory method that really checks consistency of the parameters is a good practice. But there is no need to have that in the class itself and therefor it does not need to be static. Especially if you do not take all the parameters in one method but provide a separate setter for each. The name for is Builder Pattern.

StudenBuilder{
    private  String id;
    private  String firstname;
    private  String lastname;
    private  Collection<String> documents;
    private  StudentType typeofStudent;

   public StudenBuilder withId(String id){ 
     this.id=id;
     return this;
   }
   public StudenBuilder withFirstname(String firstname){ 
     this.firstname=firstname;
     return this;
   }
   public StudenBuilder withLastname(String lastname){ 
     this.lastname=lastname;
     return this;
   }
   public StudenBuilder with(Collection<String> documents ){
     Collections.unmodifiableList(new ArrayList<String>(documents));
      return this;
   }
   public StudenBuilder withStudentType(StudentType typeofStudent ){
      this.typeofStudent=typeofStudent;
     return this;
   }
   public Student build(){
      // check consistency
      // e.g. require documents for a certain StudentType
      return new Student(id, firstname, lastname, documents, typeofStudent);
   }
}

The usage would look like this:

Student newStudent = new StudentBuilder()
                     .withId("someId")
                     .withFirstName("some first name")
                     .withLastName("some last name")
                     .withStudentType(StudentType.SOME)
                     .build();
\$\endgroup\$
  • \$\begingroup\$ I understand the convince of the builder pattern, but it doesn’t solve my issue. With the builder pattern, the client can violate the requirement that only international students require documents. With the static methods/private constructor, I prohibit the passing of documents for domestic students, and I set the enum type. I can add validation for null types in my private constructor but I left those out, to keep my question on point. My question being, is it a bad idea given my requirements and what was stated on the SESE page? \$\endgroup\$ – user150904 Oct 26 '17 at 18:30
  • \$\begingroup\$ @S.R. "but it doesn’t solve my issue. With the builder pattern, the client can violate the requirement that only international students require documents. " In opposite the builder can do this check very easily in the build() method. I updated the answer. \$\endgroup\$ – Timothy Truckle Oct 26 '17 at 20:00
  • \$\begingroup\$ With my two static methods, I don't have to do this check. This is my point, the builder in my case adds more code but little benefit. \$\endgroup\$ – user150904 Oct 26 '17 at 22:15

Your Answer

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