1
\$\begingroup\$

To recover from yesterday's disaster, I re-read the SOLID principles and refactored my code. Hopefully I did a better job than yesterday. I wanted to keep my Student class immutable, but that meant I would have to change the class every time a new Student type was introduced, that violated the S in SOLID. Here is what I came up with.

public interface StudentStatus {
    Collection<String> retrieveDocuments();
    StudentType retrieveStatus();
}

public final class Student {

   private final String ID;
   private final String firstname;
   private final String lastname;
   private final StudentStatus status;

   public Student(String iD, String firstname, String lastname, StudentStatus status) {
       ID = iD;
       this.firstname = firstname;
       this.lastname = lastname;
       this.status = status;
   }

   public Collection<String> retrieveDocuments() {
       return status.retrieveDocuments();
   }

   public StudentType retrieveStatus() {
      return status.retrieveStatus();
   }

My requirements are:

  • Domestic students don't require documentation
  • International student do require documentation (passports, etc..)

Domestic Implementation:

    public final class Domestic implements StudentStatus {

       private final StudentType type;
       private final Collection<String> documents;

       public Domestic() {
           this.type = StudentType.Domestic;
           this.documents = unmodifiableList(new ArrayList<String>(documents);
       }

       @Override
       public Collection<String> retrieveDocuments() {
           return unmodifiableList(new ArrayList<String>(documents);
       }

       @Override
       public StudentType retrieveStatus() {
           return type;
       }
   }

International Implementation:

    public final class International implements StudentStatus {

       private final StudentType type;
       private Collection<String> documents;

       public International(Collection<String> documents) {

           this.type = StudentType.International;
           this.documents = Collections.unmodifiableList(new ArrayList<String>(documents));
       }

       @Override
       public Collection<String> retrieveDocuments() {
           return Collections.unmodifiableList(new ArrayList<String>(documents));
       }

       @Override
       public StudentType retrieveStatus() {
          return type;
       }
    }

If there is any new type of status, it can be added my implementing StudentStatus, no need to modify the Student class.

Use:

        List<String> documents = new ArrayList<String>();
        documents.add("Passport");
        documents.add("Drivers License");

        StudentStatus domestic = new Domestic();
        StudentStatus international = new International(documents);

        List<Student> students = new ArrayList<Student>();
        students.add(new Student("123456789","Susan","Ceesharp", domestic));
        students.add(new Student("987654321","Bill","Finalclass", international));

        for(Student display : students) {
            System.out.println(display.getID() + " " + display.retrieveStatus());
        }

I'm aware none of my classes have validation, I kept these out to keep my code clear and concise.

Goal: Keep the Student immutable without violating SOLID.

\$\endgroup\$
0
\$\begingroup\$

You can shorten / simplify your StudentStatus implementations.

Both International and Domestic have a fixed return value for retrieveStatus(). You can leave out the type field and add the value directly in the method.

retrieveDocuments() copies what the constructor already copied. You can simply return documents. Domestic students have no documents, so retrieveDocuments() can return the empty list.

public final class International implements StudentStatus {
    private final Collection<String> documents;

    public International(Collection<String> documents) {
        this.documents = Collections.unmodifiableList(new ArrayList<String>(documents));
    }

    @Override
    public Collection<String> retrieveDocuments() {
        return documents;
    }

    @Override
    public StudentType retrieveStatus() {
       return StudentType.International;
    }
}

public final class Domestic implements StudentStatus {
    @Override
    public Collection<String> retrieveDocuments() {
        return emptyList();
    }

    @Override
    public StudentType retrieveStatus() {
        return StudentType.Domestic;
    }
}

If there is any new type of status, it can be added my implementing StudentStatus, no need to modify the Student class.

There doesn't seem to be a way to retrieve the StudentStatus from a Student. You've provided delegate methods, but if you change the StudentStatus interface, its extra methods will not be accessible unless you also change the Student class.

Now, interfaces ideally don't change often, but it's something to keep in mind.


What would be better, provide a getter that returns a StudentStatus object? Suppose if I wanted to accommodate changes to the interface, as you pointed out I provided delegate methods, but if my interfaces changes so will my student class. What should I do in this case?

The short answer is, yes, a getter will do fine.

The longer answer, much hated answer to most any question, is: It depends! (dun dun dun).

Providing a getter solves the problems that would come with adding methods to the interface. But—and there is always a but—you risk running foul of the Principle of Least Knowledge, also known as the Law of Demeter. An example will make this clearer:

  • student.retrieveDocuments() — I need to know that a student has documents. Straightforward.

  • student.getStatus().retrieveDocuments() — I need to know that a student has a status, and that the status has the documents I'm after. Not as straightforward.

StudentStatus appears to carry information that is local to a student, and is not very useful outside of the context of a student, so your instinct of 'hiding' it through delegation is a good instinct. But it makes me wonder why you have a StudentStatus in the first place.

If the goal is to close for extension and provide immutability, then the next question becomes: is StudentStatus included in the guarantee of immutability? If not, can Student be said to be immutable? That depends entirely on the relation between student and status.

tl;dr What's better depends on what you aim to achieve. Don't get mired in the perfect way to do things, because such a thing does not exist. Do think about what and why, which is what you are doing, and I wholeheartedly applaud that.

\$\endgroup\$
  • \$\begingroup\$ What would be better, provide a getter that returns a StudentStatus object? Suppose if I wanted to accommodate changes to the interface, as you pointed out I provided delegate methods, but if my interfaces changes so will my student class. What should I do in this case? \$\endgroup\$ – user150904 Oct 29 '17 at 16:58
  • \$\begingroup\$ @S.R. I added a part to the answer because you ask a very good question that deserves more than a comment. \$\endgroup\$ – JvR Oct 29 '17 at 18:19
  • \$\begingroup\$ Thanks for the update. :D I think this might be a situation where inheritance is applicable. If I want to enforce the rule that only international students have documentation and domestic student don't, I can enforce them in the respective constructors. Yes, immutability is great, but in this case inheritance is most likely the better tool. I would still have to implement new methods in my subclasses, but I don't force the client to get an object and then ask for data on that object, it works, but it's extra work that could be avoided with inheritance. \$\endgroup\$ – user150904 Oct 29 '17 at 19:11
  • \$\begingroup\$ I was also thinking that using a StudentStatus interface and delegate methods, in just attempting covering up that inheritance is probably the better tool. Any class that implements that interface would have to change, and by extent my Student class would need additional delegate methods. It seems like a poorly implemented solution to what inheritance can fix. \$\endgroup\$ – user150904 Oct 29 '17 at 23:26
0
\$\begingroup\$

As a minor note you could change this.documents = unmodifiableList(new ArrayList<String>(documents); to this.documents = Collections.emptyList();

\$\endgroup\$
  • \$\begingroup\$ Is it still immutable, clients can't add documents to domestic student, correct? \$\endgroup\$ – user150904 Oct 28 '17 at 20:43
  • 1
    \$\begingroup\$ @S.R. yes, you are correct. You can see the source code here if you're interested in more details \$\endgroup\$ – Mibac Oct 28 '17 at 20:53
  • \$\begingroup\$ is the use of interface okay? I would rather avoid abstract classes that could lead to deep inheritance. \$\endgroup\$ – user150904 Oct 28 '17 at 21:02
  • \$\begingroup\$ It looks okay to me \$\endgroup\$ – Mibac Oct 28 '17 at 21:07

Your Answer

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