Course Management with SOLID principles

Question

Trying to create a simple Course Management application using SOLID principles and best practices for Object Oriented Programming. Looking for feedback on the library I created in src/main/lib of the github project below. Example usage is shown in src/main/app if you are curious, but I am less concerned with feedback on that part.

Some requirements of the application:

• Create new courses
• Create new students
• Create new rooms
• Add students to course
• Assign room to course
• View all courses for a student
• View all students in a course
• View all courses in a room

In addition to a general code review, I am also have some specific questions:

1. What should the relationship between students and courses look like? I currently have Student containing a set of Courses, and Course containing a set of Students. This was mainly due to the "View" requirements above, but I am not sure if this is a bad practice to have the relationship going both ways.

2. For adding a Student to a Course, I have a Registrar interface with an enroll method, which should do some simple validation (does student have course at same time, is course full, etc.). The enroll method in Registrar calls student.enroll(course) and course.enroll(student), but a side-effect of this is that I needed to make a public method in Student and Course called enroll. The problem here is that I want all enrollment to go through Registrar.enroll(...), but I am wondering if it is a concern that the other enroll methods are public when they should never actually be called by anyone other than the Registrar.

Some example code (full code can be found in github):

public class Course {
    private String name;
    private Calendar time;
    private HashSet<Student> students;
    private Room room;

    public Course(String name, Calendar time) {
        this.name = name;
        this.time = time;
        this.students = new HashSet<>();
    }

    public HashSet<Student> students() {
        return students;
    }

    public void enroll(Student student) {
        students.add(student);
    }

    @Override public String toString() {
        return String.format("Course name: %s%nCourse Time: %s", this.name, this.time.getTime().toString());
    }

    public Calendar getTime() {
        return time;
    }

    public void setRoom(Room room) {
        this.room = room;
    }

    public Room getRoom() {
        return room;
    }
}

Student:

public class Student {
    private String name;
    private HashSet<Course> courses;

    public Student(String name) {
        this.name = name;
        this.courses = new HashSet<>();
    }

    @Override public String toString() {
        return name;
    }

    public HashSet<Course> courses() {
        return this.courses;
    }

    public void enroll(Course course) {
        courses.add(course);
    }
}

Registrar Implementation:

    public class RegistrarImpl implements Registrar {
    private HashSet<Course> courses = new HashSet<>();
    private HashSet<Student> students = new HashSet<>();

    @Override
    public void registerCourse(Course course) {
        courses.add(course);
    }

    @Override
    public void registerStudent(Student student) {
        students.add(student);
    }

    @Override
    public HashSet<Course> allCourses() {
        return courses;
    }

    @Override
    public HashSet<Student> allStudents() {
        return students;
    }

    @Override
    public void enroll(Course course, Student student) throws EnrollmentException {
        if(!courses.contains(course)) {
            throw new EnrollmentException("Course has not been registered.");
        } else if(!students.contains(student)) {
            throw new EnrollmentException("Student has not been registered.");
        }
        for(Course c : student.courses()) {
            if(c.getTime().getTime().getTime() == course.getTime().getTime().getTime()) {
                throw new EnrollmentException("Student already has class at this time.");
            }
        }
        course.enroll(student);
        student.enroll(course);
    }
}

Answer 1

• I don't think it is wise to make time and room a field of Course, because the validity of these fields depends on factors that are outside the scope of Course, namely whether another course is taking place at the same time in the same room. So a course storing information about when and where it takes place makes it possible to construct two Course instances with conflicting time and room values.

  Instead, you could make a class representing the total timetable that contains all courses and make this class responsible for associating courses with time-room-combinations. This gives rise to several complications:

  • The relationship between courses and time-room-combinations is bijective, which means that every course can only be associated with one time-room-combination, and every time-room-combination can only be associated with one course. A single Map would not be sufficient to represent this relationship, because a Map can contain duplicate values, so a Map<Course, TimeAndRoom> (where TimeAndRoom is a simple wrapper class that holds a time and a room) would allow multiple courses to occur at the same time and place, whereas a Map<TimeAndRoom, Course>, or a Map<Room, Map<Time, Course>>, which would probably be easier to handle, would allow one course to take place at multiple times and rooms.

    You would therefore need a class that somehow encapsulates this logic. Space-wise, the most efficient solution would probably be to store a list of time-and-room/course pairs, and make sure that no course or time-and-room combination is contained twice in this list. The price for this space-efficiency would be speed, because it would make each query or modification an \$O(n)\$ operation with regard to time complexity. A faster solution which would require more space would be storing two maps, one map for each direction. As far as I know, there are already some third-party libraries out there with classes that have been designed for this purpose, but I've never used them, so I'll leave it to you to decide whether you want to rummage through the web (you could try looking for "bijective map" or "bidirectional map" or something) or whether you want to implement this yourself.

  • Right now, your code stores the time of a course as a single point in time, but unless the length of a lesson of a course is fixed, this is not enough information, because you would need to store both the beginning and the end of the lesson. This creates the problem that an arbitrary Map<Time, Course> would not be enough to ascertain that a room does not accommodate two courses simultaneously, because the keys in a Map<Time, Course> might represent overlapping time intervals.

    A way to rectify this could be to create a custom implementation of Map<Interval<K extends Comparable<? super K>>, V> (where V can be a generic type parameter because the custom implementation only requires the keys to be of a specific type, namely an interval) that imposes the additional constraint that the intervals represented by its keys must not overlap. A Map can be easily implemented by extending AbstractMap. You could even design this custom Map implementation to be a SortedMap or NavigableMap, but I don't know if this would be too complicated to be worth it.

    You can then map every room to an instance of this custom Map implementation that stores the courses as values.

• As for the relationship between a student and a course, this is the other way round. A student can be associated with multiple courses, and a course can be associated with multiple students. Here, a single Map<Course, Set<Student>>, or a Map<Student, Set<Course>>, would be sufficient to fully and self-consistently represent this relationship. But still, you have to choose between saving space and increasing performance. If you want to make efficient look-up operations possible in both directions, you will need to create a second map in the reverse direction.

  If you do, I would suggest to encapsulate this logic in a separate class, because interdependent state like this is dangerous and an invitation for bugs, seeing as you have to pay attention that, when you make a modification to the stored data, you always need to update both maps in accordance with each other. This is probably what made you hesitant of this approach.

• Also, this is dangerous:

  public HashSet<Student> students() {
      return students;
  }
  

  You are exposing mutable state (i.e. the field students), thereby allowing your object to become corrupted from the outside. Instead, I would suggest this:

  public Set<Student> students() {
      return Collections.unmodifiableSet(students);
  }
  

  This is very handy, because it doesn't create a copy of the original set, but a read-only view, so that changes to the original set will be visible in this unmodifiable set. Note that it is necessary to change the return type of the method from HashSet to Set for this to work. In fact, declaring variables as interfaces instead of interface implementations is generally preferable anyway, because, like in this case, unless the functionality of the program depends on students being specifically a HashSet and not any kind of Set, it is more appropriate to declare students as a Set<Student> instead of a HashSet<Student>.

Answer 2

What should the relationship between students and courses look like?

Like what it is in the real world :) A course is not really a part of a student, so I would not have a Set<Course> in student.

Rather, it should be something like Enrollment, which couples a Student to a Course (maybe also at a given timeslot). And store this in the Registrar. (For example, as a Set<Enrollment> or as some Maps if performance matters)

The Registrar can check if the Student is already enrolled or occupied a a specific time.