• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Ron McLeod
  • Paul Clapham
  • Jeanne Boyarsky
  • Bear Bibeault
Sheriffs:
  • Rob Spoor
  • Henry Wong
  • Liutauras Vilda
Saloon Keepers:
  • Tim Moores
  • Carey Brown
  • Stephan van Hulst
  • Tim Holloway
  • Piet Souris
Bartenders:
  • Frits Walraven
  • Himai Minh
  • Jj Roberts

Should Course class maintained the list of enrolled student in terms of Single responsibility ?

 
Greenhorn
Posts: 6
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I am working on an assignment project, I have a student class which has a variable such as name and address. Also there is a Course class which has variable such as couseName, teacher, date, startTime and endTime. Will I be breaking the single responsibility if I add an ArraylList instance variable to the Course class to maintain the list of student that enrolled on the course? This question  arises after reading the single responsibility principle. I want to be able to print each course later and the number of students enrolled on it. And also print all the student and the number of course each student enrolled in.
 
Saloon Keeper
Posts: 7999
70
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Likes 2
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I believe that would break the principle. You'd end up with Course having a List<Student> and Student having a List<Course>. Then the problem becomes going to two places to add a Student to a Course.
You could have an Enrollment class, for example, with Map<Student,List<Course>> and also Map<Course,List<Student>>. Then you only have to go to the Enrollment object to add a Student to a Course and both maps would be maintained in sync.

Edit: Probably "Set" would be better than List.
 
Marshal
Posts: 72406
315
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Welcome to the Ranch

Carey is right. What's more, the use of a Set, as he suggested, will allow you to take some appropriate action if the same student is added twice.
 
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I don't know if I agree with you guys about this one. SRP says a class should have one and only one reason to change.  The fact that you'd have to update two different source files to maintain referential consistency and integrity of the many-to-many relationship doesn't necessarily violate the principle; it's still pretty much the same reason for change. I haven't tried it out in code but I could imagine a design that employs callbacks to maintain the relationship integrity on both ends so you could modify the list on one side and have the other side automatically be updated as well.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Famous M. Ighodaro wrote:I want to be able to print each course later and the number of students enrolled on it. And also print all the student and the number of course each student enrolled in.


This is the part that most likely will lead to breaking the SRP. Printing a class roster and printing a coarse load are different responsibilities from maintaining enrollment information. If you have methods in a Course class that maintain the roster of students and print the roster of students taking the course then that is a violation of the SRP. Likewise in a Student class if you have methods that maintain information about course load and printing the coarse load. The responsibilities of maintaining the list and printing the list should be separated. Here's why: If you need to change the way the printed list is formatted, ordered, or filtered, it has nothing to do with how the list is maintained. Maintaining the list is therefore a different responsibility from printing the list.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
So I did try it in code and it's possible to design the two classes such that two-way references are synchronized. Might be a good discussion to compare with the idea of having an Enrollment class to manage the relationship.
 
Famous M. Ighodaro
Greenhorn
Posts: 6
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Carey Brown wrote:I believe that would break the principle. You'd end up with Course having a List<Student> and Student having a List<Course>. Then the problem becomes going to two places to add a Student to a Course.
You could have an Enrollment class, for example, with Map<Student,List<Course>> and also Map<Course,List<Student>>. Then you only have to go to the Enrollment object to add a Student to a Course and both maps would be maintained in sync.

Edit: Probably "Set" would be better than List.


Thanks for your answer, I took your suggestion and this is how the Enrollment class look like now. I am not sure yet of how the enroll method should look like, but this is what I can put into work now.
 
Famous M. Ighodaro
Greenhorn
Posts: 6
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:

Famous M. Ighodaro wrote:I want to be able to print each course later and the number of students enrolled on it. And also print all the student and the number of course each student enrolled in.


This is the part that most likely will lead to breaking the SRP. Printing a class roster and printing a coarse load are different responsibilities from maintaining enrollment information. If you have methods in a Course class that maintain the roster of students and print the roster of students taking the course then that is a violation of the SRP. Likewise in a Student class if you have methods that maintain information about course load and printing the coarse load. The responsibilities of maintaining the list and printing the list should be separated. Here's why: If you need to change the way the printed list is formatted, ordered, or filtered, it has nothing to do with how the list is maintained. Maintaining the list is therefore a different responsibility from printing the list.


Hello, that was the initial idea, I mentioned it on the comment above because I want it to be taking into consideration when any one is answering my question. The SRP I was concerned about is the student and the lesson class maintaining the enrolled lessons and enrolled students respectively.
 
Carey Brown
Saloon Keeper
Posts: 7999
70
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
addLesson() and addStudent() should be private and only accessed by enroll().
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Here are some comments on the latest iteration of the code:

1. Lazy load of Enrollment instance smells of premature optimization. Just how expensive is it really to create an instance that you can justify adding complexity by lazy loading? In my opinion, this doesn't even need to be considered at this point until you have some hard data that performance is taking a hit. You have no data and even if you do, I would say the gain, if any, is insignificant. This kind of "optimization" is just the start of a spiral into a bad place and for this exercise, totally unnecessary.

2. The error field is useless: you are setting it but not using it otherwise. Again, you have just added complexity to your code with no apparent gain. Even if you add a getter for this so that it can be used, the error will only have a correct meaning if accessed immediately after calling one of the methods that sets its value. Otherwise, the semantics of error is divorced from its context. This is a very problematic design. Better to return the error but the problem with that is you're already returning a boolean to indicate whether or not the operation was successful.

3. getEnrolledStudents() and getEnrolledLessons() break encapsulation by returning direct references to private instance variables. Since these are references to mutable collections, anyone who obtains a reference can modify the contents of those collections thereby breaking the encapsulation of the Enrollment class.

4. There are misplaced responsibilities hidden in the logic of Enrollment that break the encapsulation of Course and Student respectively. I will leave it to you as an exercise. If you can't find it, I will explain at a later point in this thread. In my opinion, it is a serious violation of object-orientation because it now spreads the business logic and the responsibility of enforcing them across multiple classes. It seems there is a least one logic bug related to this.

5. The whole design and API results in awkward code. I don't know how you coded the class that uses Enrollment but here's what I imagine you could end up writing, assuming you take Carey's advice to make addLesson() and addStudent() private methods:

How is this awkward? Well, I say it's awkward because I can compare it to the code in my tests:

and

With the design and API I have, you'd write something like this:

 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
And here's my Student class implementation based on the tests I have written so far:

synchronizeEnrollment(Course) is the callback method I alluded to earlier in this thread. It has package-private (default) scope by design.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I'm holding off on posting my code for Course to give folks a chance to try to implement it given the Student class implementation and the tests. My implementation is only 18 lines of code including package declaration, import statements, and whitespace for readability. The point I'm trying to make is that with this design, the code you end up writing is smaller and relatively simpler.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Previously,

I wrote:there is at least one logic bug


The design has a number of problems related to breaking the encapsulation of Student and Lesson and misplacing the responsibility of enforcing the following rules:

1. A student can only enroll in a limited number of courses. (Apparently, no more than 2)

2. A course/lesson can only accept a limited number of students as determined by Lesson.getCapacity()

Consider what happens when a student tries to enroll in a course that is already at capacity. I haven't tested it but from what I can tell by just looking at the code, even though a student doesn't get added to a lesson because it is already at full capacity, the lesson will still get added to the student's list of lessons taken as long as they haven't yet enrolled in the maximum number of lessons allowed per student.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Coming back to the original question about SRP, the Enrollment design as it is now actually breaks SRP in that there are at least two reasons for changing the Enrollment class:

1. If you change the rules for accepting a student to a course. Currently, the rule is that the course must not be at full capacity.

2. If you change the rules for allowing a student to enroll in a course. Currently, the rule is that the student can enroll in a maximum of two courses.

I would consider an alternative design where you introduce the concept of a "Policy". This way, you can have a StudentMaxLoadPolicy and a CourseMaxCapacityPolicy be enforced somehow before you add a student to a course roster and a course to a student's load.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
This kind of code is also a violation of the principle of encapsulation:

The symptom related to this violation is the duplication of the logic on lines 109-110.
 
Saloon Keeper
Posts: 4346
163
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Just starting reading this topic. Interesting!

My first impressions are:

1. I like that enrollment class (haven't quite understood the objections sofar, but I'll give it anoher thorough read). I do not like to mutate a Lesson and a Student whenever a Lesson-Student is added.

2. Too much information is recorded. What more than a Map<Lesson, Set<Student>> is required? It is easy to derive any information from this map.

3) OP's addLesson and addStudent can be quite reduced in size, for instance by using 'map.computeIfAbsent'.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Piet Souris wrote:
2. Too much information is recorded. What more than a Map<Lesson, Set<Student>> is required? It is easy to derive any information from this map.


I think this is a good call out. Thinking back to Peter Coad's designing in color archetypes, he has a Moment-Interval archetype that captures a temporal relationship between entities. Course-Student is such as relationship because it exists only for a finite interval of time. You could conceivably simplify it further to be just a collection of (Course, Student) tuples. Having a Map<Lesson, Set<Student>> or Map<Student, Set<Lesson>> is really just an optimization.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Piet Souris wrote:
1. I like that enrollment class (haven't quite understood the objections sofar, but I'll give it anoher thorough read). I do not like to mutate a Lesson and a Student whenever a Lesson-Student is added.


Let's dig into this a little more. I think I have an idea as to why you don't like this but as good developers, we have to articulate our reasons for choosing one design over another beyond just liking or not liking it. I could come up with a few ways to reason against the design I showed as well but let's talk about your reasons first.
 
Piet Souris
Saloon Keeper
Posts: 4346
163
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:You could conceivably simplify it further to be just a collection of (Course, Student) tuples. Having a Map<Lesson, Set<Student>> or Map<Student, Set<Lesson>> is really just an optimization.


Yes, you are correct. But a Map is the first thing that comes up with me when I read the assignment.
 
Piet Souris
Saloon Keeper
Posts: 4346
163
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Let's dig into this a little more. I think I have an idea as to why you don't like this but as good developers, we have to articulate our reasons for choosing one design over another beyond just liking or not liking it. I could come up with a few ways to reason against the design I showed as well but let's talk about your reasons first.


Sorry about the delay.

I have the picture in mind that a Lesson (or Course) should be something that is as stable as possible. For instance: you have the Lesson "Math for third years", containing a list of subjects and requirements about what a Student is supposed to be capable of, once done. That should hardly change.
Then there are the concrete versions of this, with who is the teacher, on what dates and times it is given, a set of students, et cetera. As said, I would store that in a separate class.

I've looked at your code, with a Lesson having a List<Student> and a Student having a List<Lesson>, and a very elegant way to secure the synchronicity between them. So, for me it is just what feels best. Also a factor: I like deriving information from a Map, how irrelevant that may be.
 
Carey Brown
Saloon Keeper
Posts: 7999
70
Eclipse IDE Firefox Browser MySQL Database VI Editor Java Windows
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
One reason I gravitate to using an Enrollment class is it more closely matches a database implementation of a table with two columns and two indexes. So a migration to a database for persistence would be relatively painless.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Piet Souris wrote:
I have the picture in mind that a Lesson (or Course) should be something that is as stable as possible. For instance: you have the Lesson "Math for third years", containing a list of subjects and requirements about what a Student is supposed to be capable of, once done. That should hardly change.
Then there are the concrete versions of this, with who is the teacher, on what dates and times it is given, a set of students, et cetera. As said, I would store that in a separate class.


I actually like this perspective because it aligns with the Person, Place, or Thing archetype from Coad's four archetypes. I would probably try refactoring my initial design to improve the conceptual integrity so that Student and Course are more stable and would just delegate to a Moment-Interval archetype Offering to track Student rosters (a specific offering of a Course, unique by [Instructor, Term, Schedule])

Then, all you really need is to maintain a Set<Offering> to get class rosters using streams, collectors, and grouping by Offering. By the same token, you can also query the Set<Offering> to get each student's set of class offerings taken for a particular term. The mechanism to keep parallel collections synchronized wouldn't even be necessary, which just goes to show that it was really just another premature optimization that only made the code and design more complex.
 
Junilu Lacar
Sheriff
Posts: 16103
268
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Carey Brown wrote:One reason I gravitate to using an Enrollment class is it more closely matches a database implementation of a table with two columns and two indexes. So a migration to a database for persistence would be relatively painless.


This a more traditional design approach; nothing wrong with it. If you switch the paradigm to an event-driven design where you just store the event stream, the Set<Offering> I describe in my last post will be a better fit for persisting.
 
Piet Souris
Saloon Keeper
Posts: 4346
163
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:I actually like this perspective because it aligns with the Person, Place, or Thing archetype from Coad's four archetypes (...)
Then, all you really need is to maintain a Set<Offering> to get class rosters using streams, collectors, and grouping by Offering. By the same token, you can also query the Set<Offering> to get each student's set of class offerings taken for a particular term. The mechanism to keep parallel collections synchronized wouldn't even be necessary, which just goes to show that it was really just another premature optimization that only made the code and design more complex.


Thanks for the link!

Can you go on with your design? I am very curious (you ARE the master of making complex things look easy). What about the exam results of a Student for Course X?

And hopefully OP will give some feedback about his assignment.
 
Famous M. Ighodaro
Greenhorn
Posts: 6
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:Previously,

I wrote:there is at least one logic bug


The design has a number of problems related to breaking the encapsulation of Student and Lesson and misplacing the responsibility of enforcing the following rules:

1. A student can only enroll in a limited number of courses. (Apparently, no more than 2)

2. A course/lesson can only accept a limited number of students as determined by Lesson.getCapacity()

Consider what happens when a student tries to enroll in a course that is already at capacity. I haven't tested it but from what I can tell by just looking at the code, even though a student doesn't get added to a lesson because it is already at full capacity, the lesson will still get added to the student's list of lessons taken as long as they haven't yet enrolled in the maximum number of lessons allowed per student.


Thanks for the feedback and thorough analysis of my code above line by line. I found out this bug a few days back and this is how I handled it. Please I any critique is always welcome. I added a removeStudent method to handle it.



 
Campbell Ritchie
Marshal
Posts: 72406
315
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Why have you made the Enrollment class a singleton? I don't think that is the correct way to write a singleton anyway.
 
Famous M. Ighodaro
Greenhorn
Posts: 6
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Junilu Lacar wrote:And here's my Student class implementation based on the tests I have written so far:

synchronizeEnrollment(Course) is the callback method I alluded to earlier in this thread. It has package-private (default) scope by design.


I like how you simplified your code. My concern is that, the student class is maintaining a list of enrolled lessons and also updating the lesson object by adding a student to the new course. Won't this be breaking single responsibility principle or I am getting the idea of single responsibility principle all wrong? I am still learning, so explanation if what I am getting wrong will be appreciated.
 
Famous M. Ighodaro
Greenhorn
Posts: 6
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:Why have you made the Enrollment class a singleton? I don't think that is the correct way to write a singleton anyway.


The reason of the Singleton is that the enrollment class is holding the list of enrolled students and lessons. To print out the list of enrolled students and lessons later from another class will possible by calling just one instance of the enrollment class.
I choose this way of writting Singleton because the assignment is not a muiltithreaded application.
 
Campbell Ritchie
Marshal
Posts: 72406
315
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Both, I am afraid, look incorrect. Making it a singleton means you can only have one college conceptually. Make it an ordinary class and instantiate it once.
Using not quite best quality code because you aren't multi‑threading looks like a good way to get into bad habits, which will take a very long time to break.
 
So it takes a day for light to pass through this glass? So this was yesterday's tiny ad?
SKIP - a book about connecting industrious people with elderly land owners
https://coderanch.com/t/skip-book
reply
    Bookmark Topic Watch Topic
  • New Topic