I'm lately trying to implement a specific problem using an object-oriented approach. I get the main principles and its advantages, but I fail to apply it to a real world problem. Most examples one could find consist of Cats
or Dogs
being implementations of Animal
. These however don't give me enough understanding how to model below problem regarding another frequent example: a school administration system.
Imagine a school having Students
, Courses
, Professors
, and Notes
.
My implementation would be something like this:
class Person {
string name;
int age;
Person(string name, int age) {
this.name = name;
this.age = age;
}
}
class Student extends Person {
double gpa;
Student(string name, int age) {
super(name, age);
}
}
class Professor extends Person {
string roomNumber;
Professor(string name, int age, string roomNumber) {
super(name, age);
this.roomNumber = roomNumber;
}
}
class Course {
string name;
Professor professor;
Students[] student;
Course(string name, Professor professor) {
this.name = name;
this.professor = professor;
this.students = new Student[];
}
void enrolStudent(Student student) {
students.add(student);
}
}
class Note {
Course course;
Student student;
double value;
Note(Course course, Student student, double value) {
this.course = course;
this.student = student;
this.value = value;
}
}
Now the Student
has a bunch of Notes
and we want to calculate its GPA. This could be either straightforward averaging its Notes
' values or more complex logic using weights and/or ignoring optional courses.
Now my question is: where do we put this logic? Ideally I would have a function double calculateGpa()
on Student
so you could call student.calculateGpa()
, but having this logic on Student
would break the SRP in my view. It also does not belong to any other class listed here. A class called GpaCalculator
or NotesManager
would be another guess but that seems to me too much like moving all the logic away from the domain and into classes that do not represent a real object but just actions (see also this answer).
If that would be the way to go here, why wouldn't I then just write a pure, static, stateless function in a class called NotesHelper
? Creating a manager class to just have one function double calculate()
, and using its instance instead of a static function feels to me like making it look like OOP while it isn't really. I feel like there should be a better approach, probably one I didn't think of, or maybe I am wrong here. Could you guys give me some pointers?
8 Answers 8
Indeed, many examples of inheritance are trivial and even misleading. Consider this - it's true that Professors and Students are both Persons, but does your program need to know that? As it stands, this is just unnecessary coupling between the classes.
Software is "soft" because it changes easily. Relatively, anyways. I assume you are not publishing a library, but an application with a user interface (graphical or otherwise). In this case, you can be flexible. No one else needs to know where the calculateGpa
function is. If it ends up being too complex for Student
, or changing too often, you can extract it then. Hindsight is 20/20, and in software we can adapt to that!
There is no "one true way" to model a domain - it always depends on which problem(s) you are trying to solve. But you probably don't know what problem you're solving, yet, so you should do the minimum viable thing that gets you closer to knowing the problem. The best design - at least for software you are maintaining - is usually one that is easy to change, not one that is maximally flexible. Being small helps a lot. So stay small until you have a reason not to.
Just put it in Student for now. Move it later, or don't.
My simple take on this is that GPA is a property of a student. Hence it should be calculated in the Student
class. Student
doesn't currently have any responsibilities — it's a pure data class — so I don't see how it breaks SRP.
Also:
- It seems
calculateGpa
should be simplygpa
, replacing the field. - There is a many-to-many connection between
Student
andCourse
, so it seems there is a missingEnrolment
intermediary class holding theNote
(which then no longer needs to be a class, because there's just thevalue
field left).
-
1But whenever the Student model changes, we should change the
Student
class. Now imagine that we add a propertyweight
toNote
, or toCourse
. Should theStudent
class be responsible for the calculation of the notes then? These are 2 different reasons for changing theStudent
implementation; isn't that breaking SRP?Steven.B– Steven.B07/10/2019 11:40:11Commented Jul 10, 2019 at 11:40 -
2
Should the Student class be responsible for the calculation of the notes then?
-- Why would it be? You just said that those properties belong to other classes.Robert Harvey– Robert Harvey07/10/2019 15:12:07Commented Jul 10, 2019 at 15:12 -
Your model will evolve; I'm only talking one step removed from the current model. At some point it may be natural, for example, that the
School
should do the GPA calculation, because that is what actually happens in real life. And it sounds like weights belong to aCourse
, which might move your model in that direction. If you try to think about the optimal shape of the current model and the optimal shape of the finished system you will often find that they contradict. That's what automated tests and refactoring is for.l0b0– l0b007/10/2019 20:11:59Commented Jul 10, 2019 at 20:11 -
calculating the GPA is a function of the school administration, not the student. Thus it belongs in some service class that calculates grades. The student class should defer the actual calculation to there, and merely store the result of the calculation.jwenting– jwenting07/18/2019 08:37:32Commented Jul 18, 2019 at 8:37
In situations like this, I think it's less helpful to dwell on abstract models of the domain (domains can be modelled very differently depending on perspective) and more consider the work that needs to be completed and focus on encapsulation and - more specifically - the concept of tell, don't ask.
If I want to calculate a GPA, it's clearly not the Student's job to know how to do that. So I want a GPACalculator
*, which I'm going to pass to the Student. The student can pass that through its Note
s and each Note
can report itself to the GPACalculator
, which will calculate the GPA according to some algorithm that both Student
and Note
s are ignorant of.
Something along the lines of:
public class Student {
private List<Note> notes;
public void CalculateGpa(GPACalculator calculator) {
foreach(var note in notes) {
note.ReportValue(calculator);
}
}
}
public class Note {
private double value;
public void ReportValue(GPACalculator calculator) {
calculator.AddNoteValue(value);
}
}
public class GPACalculator {
private List<double> values;
public void AddNoteValue(double value) {
values.Add(value);
}
}
Then, you simply pass your calculator to your student and end up with a fully populated GPACalculator
(either storing the note values as above and then calculating at the end, or calculating as you go, depending on what makes the most sense algorithmically).
Your GPACalculator
is ignorant of your Student and its properties, your Student is ignorant of your GPA calculation.
When you want to retrieve your GPA, you can either expose a property from the GPACalculator
or (better) have another method that allows you to pass another object for your GPACalculator
to report its calculated GPA to.
"Tell, Don't Ask" is, for me, the essence of good OOP.
It's of course very important to remember that the storage model and the domain model are not necessarily the same as the concerns for the two things are very different.
(* In honesty, I would be tempted to ditch the "calculator" suffix and just call this class Gpa
.)
-
Could you elaborate what you mean by tell, don't ask in this example? Also: I like the idea to just call it GPA.Steven.B– Steven.B07/17/2019 13:33:33Commented Jul 17, 2019 at 13:33
-
@Steven.B "Tell, Don't Ask" is basically colloquialism for encapsulation, i.e. having data be encapsulated within the same class that operates on the data. In practise this encourages avoiding exposing values to a caller via properties and instead having the component keep its data private. e.g. instead of fetching
item.Foo
and then callingConsole.WriteLine
, you'd pass aConsoleWriter
toitem
'sReportFoo
method, thenitem
would callwriter.Write(foo)
and not exposeFoo
as a property at all.Ant P– Ant P07/17/2019 14:10:59Commented Jul 17, 2019 at 14:10 -
1In the example above, this manifests itself as the caller passing the GPA to the student, which passes it through the Notes and each Note tells its value to the GPA, keeping the GPA ignorant of the implementation of Notes and Students and the Notes and Students ignorant of the GPA algorithm. As opposed to, e.g., the caller looping through the Notes to construct the GPA, or the GPA looping through the Notes and asking for their values.Ant P– Ant P07/17/2019 14:16:24Commented Jul 17, 2019 at 14:16
I agree with Engineert on the have a property and delegate the calculation thing. Some remarks on your model:
You start with Person as a base class. You should ask yourself why. Yes, both students and teachers are people but is that relevant to the problem? OK, you have some common properties but will you ever in your application have to deal with people? Probably not.
You have a list of students in Course and a teacher. This seems wrong. During some time period/semester there will be a relationship between students and a particular course and one teacher will be teaching the course but students nor teachers are properties of the course. You are still missing a couple of classes here. One of those is a roster/schedule. This should reference students and courses and teachers, bring them together. For each period you will have new instances of roster, the course does not need to know about either (teachers, students or rosters).
This may be the most important question in OO development that you will be asking yourself a lot: "Does my class need to know about this?" Is it essential to its identity? Is it part of the things it needs to be a YourClassName ?
The problem I see is that you didn't design a model to reflect domain-specific behaviours. Instead, you did focus mainly on the data structures oriented to their persistence as an ER model.
While ER models' main goal is efficiency at doing CRUD, they are horrible at encapsulating invariants and domain-specific rules because most of the time we have to break encapsulation to make these data structures integrable with tools and libs. This sort of classes makes us break many of the OOP best practices what lead us to confusion regarding the logic and its location.
ER models also fail at describing responsibilities, abstractions and the relationship between these abstractions. Think about the relationship Student
- GPA
. What does the GPA
say about the Person
represented as Student
? How GPA
describe what Student
s are or how they behave? Is GPA
to a Student
what X, Y
is to a Point
? Does Student
depend on GPA
? Really? While the relationship suggested could make sense from the persistence standpoint, it makes very little from the domain.
So, what are we missing here? The GPA
Grade Point Average itself, but as a first-class citizen of the domain instead of a mere integer conveniently misplaced somewhere else.
The GPA
is not a score. The score is its current state or its representation. The GPA
is a formula. An aggregate that depends on the Student
' Notes
. A GPA
probably doesn't make sense without Notes
and Student
s, but these both makes sense without GPA
.
Regarding changes, Student
, Notes
and the GPA
are likely to change due to different reasons, due to different actors. What makes Student
to change has nothing to do with the GPA
. And vice-versa. Modifying the Student
should not involve (in no way) the GPA
. Modifying the GPA
should not involve the Student
or the Notes
it belongs to.
If GPA
and Student
change and evolve independently, it's persistence can be independent too. While GPA
is stored in memory as a function, on DB it can be stored as a score if we need to query by scores. If we don't, the GPA doesn't need to be persisted because as an aggregate, it can be calculated on-the-fly in run-time by fetching Notes
.
I think you are confused about SRP. "GPA is a property of a student" as @l0b0 mentioned and Student
class should keep this value. But it doesn't mean you should calculate GPA in this property. You can call another service method instead. When you do this, Student class can not be effected by changing calculation of GPA. It will be responsible of its properties.
public class Student
{
private GPAHelper _gPAHelper;
public Student(int studentId)
{
_gPAHelper = new GPAHelper();
_studentId = studentId;
}
public Student(int studentId, GPAHelper gPAHelper)
{
_gPAHelper = gPAHelper;
_studentId = studentId;
}
private int _studentId;
public int StudentId
{
get
{
return _studentId;
}
}
public double GPA
{
get
{
return _gPAHelper.calculateGPA(_studentId);
}
}
}
When you call a service class or a helper class, you give the responsible to this class. Calling helper or service class in Student
class does not mean you give the responsibility of GPA calculation to Student
. That's the point.
Someone doesn't like passing GPAHelper
to Student
class. I added another constructor.
-
1Agree.Calculating a GPA is a business function that could become very complex over time. It belongs it its own class.GHP– GHP07/10/2019 14:39:35Commented Jul 10, 2019 at 14:39
-
If Student is the only class that ever needs to calculate a GPA, having a separate GPAHelper class is just unnecessary complication and indirection. And now all of your calling code needs to deal with Students and GPAHelpers rather than just Students. At that point, don't even bother passing a GPAHelper to the Student. Just have the caller calculate the GPA themselves since they already have instances of the Student and GPAHelper.17 of 26– 17 of 2607/10/2019 16:43:11Commented Jul 10, 2019 at 16:43
-
@17of26 Let's consider Customer class then. Will you perform all e-commerce operation on Customer class because of just customers purchase something ?Engineert– Engineert07/12/2019 06:28:37Commented Jul 12, 2019 at 6:28
-
This post got 4 times upvote and 3 times downvote. Is it normal ?Engineert– Engineert07/18/2019 09:03:45Commented Jul 18, 2019 at 9:03
A student is a person. It makes sense for Student
to inherit from Person
.
A professor is a person. It also makes sense for Professor
to inherit from Person
.
But a professor could also take classes, therefore a professor can also be a student. The inheritance paradigm of Student extends Person
and Professor extends Person
breaks down here.
If you separate the "Person" from the "Student" and "Professor" concepts you can have a professor who is also a student. This also helps solve your problem of where to put student logic.
A Student is the marriage of a Person and zero or more Courses. Composition is your friend here.
A Student is composed of a Person object and a collection of Course objects.
The GPA is a calculation of the cumulative grades received from all of the courses, including courses that are in progress. I would not say GPA is a property of student, but a function of the Student object. It should be a method.
The calculateGpa()
method loops over all of the courses and tallies up scores for courses completed, as well as marks received in all of the student's courses that are currently under way, then returns the average of this calculation.
class Student
{
private Person person;
private Collection<Course> courses;
// constructors
public double calculateGradePointAverage() {
if (courses.getCount() == 0)
return 0;
double sum = 0;
for (Course course : courses) {
// sum up grades and marks on assignments
}
return sum / courses.getCount();
}
}
-
1A question : why do you calculate gpa in student class? Because of just students have gpa ? If yes, assume that you need to log every single gpa calculation. Will you add this log also here ?Engineert– Engineert07/10/2019 13:11:39Commented Jul 10, 2019 at 13:11
-
1@Engineert: Logging is a cross cutting concern that doesn't belong in the Student class. Many tech stacks have logging frameworks that utilize aspect oriented programming ("attributes" in .NET and "annotations" in Java, for example). That would be a more appropriate solution for logging, while still keeping the purity of the Student class intact.Greg Burghardt– Greg Burghardt07/10/2019 13:24:23Commented Jul 10, 2019 at 13:24
-
For something that's explicitly an average, I'd prefer something like
return courses.Average(course => course.grade(this));
Caleth– Caleth07/11/2019 11:16:58Commented Jul 11, 2019 at 11:16 -
@Caleth: That's fine. The implementation in this case is not relevant further down than the
calculateGradePointAverage()
method. The whole point is to abstract this calculation away into an object that has all the information it needs to return the proper number.Greg Burghardt– Greg Burghardt07/11/2019 11:47:58Commented Jul 11, 2019 at 11:47
Disclaimer
I'm afraid there is no "right" or "wrong" here. There are just a bunch of opinions, and either way you will be able to find a sizeable group of people supporting the idea. As such, my answer here is not a "this is correct", but rather a "this works for me".
A direct answer to your question(s)
why wouldn't I then just write a pure, static, stateless function in a class called NotesHelper? Creating a manager class to just have one function double calculate(), and using its instance instead of a static function feels to me like making it look like OOP while it isn't really.
I agree and disagree at the same time.
It is correct that there is no difference concerning functionality. In this situation, a method or a static function will do the same thing, and apart from having to instantiate the class, there is no difference in usage either.
On the other hand, the OOP concept of encapsulation means caring about the interface and not about the implementation. All that we care about is that we call gpaCalculator.Calculate(...)
to get the GPA. What GpaCalculator
does internally - or if there even is an "internally" - doesn't matter to us. The class GpaCalculator
does not have any fields and it could be a static function instead? We don't care.
To what end though?
The benefits of using an object here is in the encapsulation. This doesn't give you any advantage at the moment. It's more about the future.
Imagine the calculation gets more complicated in the future and GpaCalculator
now does need a private field to do its job. You can add it with a constructor argument; now every place where it's constructed gives you an error and tells you "hey, I need that thing over here". This might only be one place.
However, every place where Calculate()
is called will not need to change. And that makes sense, because those calling places haven't changed; they still just want to have the GPA calculated.
(Maybe GPA calculation doesn't change and it's a bad example, I don't really know the US school system. The general idea should be clear though.)
Additional info
Managers
Please put a lot of effort into avoiding "-manager" as a class name. It doesn't tell you anything about what it does, really. Just compare your own example; imagine how much another programmer knows if they see the class name GpaCalculator
vs. NotesManager
. Use a thesaurus, it's the most underrated programming tool.
Inheritance
Inheritance is way over-hyped in schools. I would advise to favour composition over inheritance. So, a Student
and a Professor
"have" a Person
, instead of "being" one. It doesn't make a lot of sense in English, but it will save you some headaches. For example, a professor might take a course. You can then easily have a Professor
and a Student
both referring to the same Person
object.
The "Uncle Bob" school of OOP
In the book "Clean Code", Robert Martin elaborates on a style of OOP that has worked well for me, and which I still use:
Objects hide their data behind abstractions and expose functions that operate on that data. Data structure[sic] expose their data and have no meaningful functions.
Well, "data structures" already has a different meaning in computer science, so I prefer to call them "data holders". Either a class holds data (in your example, Student
, Professor
, Course
etc.), or it does something to data (e.g. GpaCalculator
). When I say that this "works well for me", I mean it provides
- easier creation; it's pretty clear where new code goes, less issues with finding good names etc.
- easier change; it keeps things pretty easy to refactor (e.g. a change in
GpaCalculator
is pretty much unable to introduce errors in other objects that work with the same data) - easier diagnosis; bugs are relatively easy to track down
Some people don't like this approach, but that seems to be restricted to people who haven't used it.
-
"Either a class holds data (in your example, Student, Professor, Course etc.), or it does something to data (e.g. GpaCalculator)" - AgreeGHP– GHP07/11/2019 15:41:15Commented Jul 11, 2019 at 15:41
Student
model are 2 different reasons for rewriting theStudent
class. Should I update my question to include for example weights on Courses or on Notes? That would maybe make more clear why I'm hesitating adding calculation logic toStudent
.Student
orPerson
, nor is there any requirement for "OO" design to involve mixing data with behaviour in the same class. Perhaps yourStudent
andPerson
classes should be pure data entities with no behaviour, used for persistence and data modelling instead?new GPA(...)
... Or you know, whatever works for you and looks intuitive, testable, and extensible to you and your team...