6

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?

  • Why do you think having student grade logic in a student class breaks SRP? – Dan Wilson Jul 10 at 10:09
  • Because I thought that, changing the logic of the calculation of the GPA and for example changing the Student model are 2 different reasons for rewriting the Student 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 to Student. – Steven.B Jul 10 at 11:44
  • 1
    @Steven.B I completely agree with your thought there; IMHO currently the responsibility of a student is "defines what data a student has". Even if you don't see that as an own responsibility and add GPA calculation to it, you end up with "A student is responsible for calculating a GPA", which sounds weird. You only have to wait for the next bit of functionality to undeniably end up at multiple responsibilities. E.g. sending an email to the student, you'll have "A student is responsible for calculating a GPA and sending emails". – R. Schmitz Jul 10 at 14:25
  • 2
    It seems to me as if you are conflating the issues of data modelling and Object-Oriented Programming. Remember that OO is all about modelling behaviour rather than data. As far as "OO" programming is concerned, there's absolutely no need whatsoever to have classes which are named after real-world things like Student or Person, nor is there any requirement for "OO" design to involve mixing data with behaviour in the same class. Perhaps your Student and Person classes should be pure data entities with no behaviour, used for persistence and data modelling instead? – Ben Cottrell Jul 11 at 11:12
  • 1
    new GPA(...) ... Or you know, whatever works for you and looks intuitive, testable, and extensible to you and your team... – svidgen Jul 12 at 0:37
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.

3

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 simply gpa, replacing the field.
  • There is a many-to-many connection between Student and Course, so it seems there is a missing Enrolment intermediary class holding the Note (which then no longer needs to be a class, because there's just the value field left).
  • 1
    But whenever the Student model changes, we should change the Student class. Now imagine that we add a property weight to Note, or to Course. Should the Student class be responsible for the calculation of the notes then? These are 2 different reasons for changing the Student implementation; isn't that breaking SRP? – Steven.B Jul 10 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 Jul 10 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 a Course, 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 Jul 10 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 Jul 18 at 8:37
2

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 Notes and each Note can report itself to the GPACalculator, which will calculate the GPA according to some algorithm that both Student and Notes 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 Jul 17 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 calling Console.WriteLine, you'd pass a ConsoleWriter to item's ReportFoo method, then item would call writer.Write(foo) and not expose Foo as a property at all. – Ant P Jul 17 at 14:10
  • 1
    In 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 Jul 17 at 14:16
1

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 ?

1

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 Students 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 GPAGrade 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 Students, 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.

1

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.

  • 1
    Agree.Calculating a GPA is a business function that could become very complex over time. It belongs it its own class. – Graham Jul 10 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 Jul 10 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 Jul 12 at 6:28
  • This post got 4 times upvote and 3 times downvote. Is it normal ? – Engineert Jul 18 at 9:03
0

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();
    }
}
  • 1
    A 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 Jul 10 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 Jul 10 at 13:24
  • For something that's explicitly an average, I'd prefer something like return courses.Average(course => course.grade(this)); – Caleth Jul 11 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 Jul 11 at 11:47
0

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)" - Agree – Graham Jul 11 at 15:41

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.