University Grading System (OOP)

Question

I've been furthering my development and understanding in OOP (Object Oriented Programming) and would appreciate feedback on this new console program I've created. The main aspects I would like to know are have I made good use of OOP, can I improve upon techniques demonstrated in this program? Have I done anything bad? I do understand I should be loading data from a text-file, but that's not the point of this program. For quick run of the program I've put sample data which is generated in the "University" constructor. I also understand that's probably not the best name to call my class. I based this program off the following scenario. Many thanks to anyone who gives me any feedback!

// Progress of Students Calculator.cpp : This file contains the 'main' function. Program execution begins and ends there.
#include <iostream>
#include <vector>
#include <algorithm>
#include <numeric>
#include <map>
class User 
{
    static int currentID;
    int m_ID;
    std::string m_Name;
public:
    User(std::string name) : m_Name{ name }, m_ID {currentID++}{
}
    int ID() const { return m_ID; }
    std::string name() const { return m_Name; }
};
int User::currentID = 1;
class Subject {
    static int currentID;
    int m_ID;
    std::string m_Name;
public:
    Subject(std::string name) :m_ID{ currentID++ }, m_Name{ name }{}
    int ID()const { return m_ID; }
    std::string name()const { return m_Name; }
};
int Subject::currentID = 1;
class Department {
    static int currentID;
    int m_ID;
    std::string m_Name;
public:
    Department( std::string name) :m_ID{ currentID++ }, m_Name{ name }{}
    int ID()const { return m_ID; }
    std::string name()const { return m_Name; }
};
int Department::currentID = 1;

class Lecturer : public User {
    int m_SubjectID;
    int m_DepartmentID;
public:
    Lecturer(std::string name, int subjectID, int departmentID) 
        :User(name), m_SubjectID(subjectID), m_DepartmentID(departmentID) {}
    int subjectID()const { return m_SubjectID; }
    int departmentID()const { return m_DepartmentID; }
};

class Student : public User {
    int m_ClassID;
    int m_Result;
public:
    Student(std::string name, int classID, int result)
        :User(name), m_ClassID(classID), m_Result(result) {}
    int classID() const { return m_ClassID; }
    int result() const { return m_Result; }
   
};

class Class 
{
    static int currentID;
    int m_ID;
    int m_LecturerID;
    int m_Size;
    int m_TotalScore;

public:
    Class(int lecturerID) 
        :m_ID{ currentID++ }, m_LecturerID{ lecturerID }, m_Size{ 0 }, m_TotalScore{0}
    {
    }
    int ID()const { return m_ID; }
    int lecturer_ID() const { return m_LecturerID; }
    int size() const { return m_Size; }
    void increment_size() { m_Size++; }
    int total_score() const { return m_TotalScore; }
    void total_score(int result) { m_TotalScore += result; }
};
int Class::currentID = 1;

    class University {
    private:
        std::vector<Department>objDepartments;
        std::vector<Subject>objSubjects;
        std::vector<Lecturer>objLecturers;
        std::vector<Student>objStudents;
        std::vector<Class>objClasses;

        auto find_lecturer_by_id(int ID) {

            return std::find_if(objLecturers.begin(), objLecturers.end(), [&](Lecturer lecturer) {return lecturer.ID() == ID; });
        }
        auto find_subject_by_id(int ID) {

            return std::find_if(objSubjects.begin(), objSubjects.end(), [&](Subject subject) {return subject.ID() == ID; });
        }
        auto find_department_by_id(int ID) {
            return std::find_if(objDepartments.begin(), objDepartments.end(), [&](Department department) {return department.ID() == ID; });
        }
        auto find_class_by_id(int ID) {
            return std::find_if(objClasses.begin(), objClasses.end(), [&](Class objClass) {return objClass.ID() == ID; });
        }
    public:
        University() {
            objSubjects.emplace_back(Subject{"Programming" });
            objSubjects.emplace_back(Subject{"Web App Development" });
            objSubjects.emplace_back(Subject{"Networking" });
            objDepartments.emplace_back(Department{"Winter Gardens" });
            objDepartments.emplace_back(Department{"South West Skills Campus" });
            objLecturers.emplace_back(Lecturer{"Sean Shearing", 1, 1 });
            objClasses.emplace_back(Class{ 1 });
            objClasses.emplace_back(Class{ 1 });

            //Test data
            add_student("Jack Cole", 1, 56);
            add_student("Sam Cole", 1, 78);
            add_student("Devon Phillips", 1, 91);
            add_student("Thomas Saunders", 1, 54);
            add_student("Jack Kimmins", 2, 58);
            add_student("Oli Stahmer", 2, 91);
            add_student("George Bradley", 2, 78);   
            add_student("Joshua Price", 2, 54);                
        }

        auto best_performing_class() {
            return std::max_element(objClasses.begin(), objClasses.end(), [&](const Class& lhs, const Class& rhs)
                {
                    return lhs.total_score() < rhs.total_score();
                });
        }
        auto worst_performing_class() {
            return std::min_element(objClasses.begin(), objClasses.end(), [&](const Class& lhs, const Class& rhs)
                {
                    return lhs.total_score() < rhs.total_score();
                });
        }
        double average(std::vector<Student>& students) {
            
            auto sum = std::transform_reduce(students.begin(), students.end(), 0.0, std::plus<>(),
                [](auto& student) { return student.result(); });
            return sum / objStudents.size();
        }
        auto find_students_in_class(int classID) {
            std::vector<Student>students;
            for (const Student& objStudent : objStudents) {

                if (objStudent.classID() == classID) {
                    students.push_back({ objStudent });
                }
            }
            return students;
        }

        int class_median_score(int class_id) {
            std::vector<int>scores;
            auto students = find_students_in_class(class_id);
            for (Student& student : students) {
                scores.push_back(student.result());
            }
            std::sort(scores.begin(), scores.end()); 
            if (scores.size() % 2 == 0) 
            {
                return (scores.at(scores.size() / 2  - 1) + scores.at(scores.size()/2)) / 2;                        
            }
            return scores.at(scores.size()/2);
        }
        void view_class_students(int classID) {
            auto students = find_students_in_class(classID);
            display_students(students);
            
        }
        void display_students(std::vector<Student>& objStudents) {
            for (const Student& objStudent : objStudents) {
                std::cout << "ID: " << objStudent.ID() << " | ";
                std::cout << "Name: " << objStudent.name() << " | ";
                std::cout << "Score: " << objStudent.result() << "\n";
            }
        }
        auto highest_mark_by_student(std::vector<Student>& objStudents) {
            return std::max_element(objStudents.begin(), objStudents.end(), [&](const Student& lhs, const Student& rhs)
                {
                    return lhs.result() < rhs.result();
                });
        }
        auto lowest_mark_by_student(std::vector<Student>& objStudents) {
            return std::min_element(objStudents.begin(), objStudents.end(), [&](const Student& lhs, const Student& rhs)
                {
                    return lhs.result() < rhs.result();
                });
        }
       
        void class_report(int class_id) 
        {
            auto students = find_students_in_class(class_id);
            auto highestScorer = highest_mark_by_student(students);
            auto lowestScorer = lowest_mark_by_student(students);
            std::cout << "Highest scorer: Student ID: " << highestScorer->ID() << " | Name: " << highestScorer->name() << " | score: " << highestScorer->result() <<"\n";         
            std::cout << "Lowest scorer: Student ID: " << lowestScorer->ID() << " | Name: " << lowestScorer->name() << " | score: " << lowestScorer->result() << "\n";
            std::cout << "Median score: " << class_median_score(class_id) << "\n";
            std::cout << "Average score: " << average(students) << "\n";
        }

        void overall_classes_report() {
            auto bestClass = best_performing_class();
            auto worstClass = worst_performing_class();
            std::cout << "\nBest Performing Class(s)...\n";
            std::cout << "ID: " << bestClass->ID() << " | Lecturer: " << find_lecturer_by_id(bestClass->lecturer_ID())->name() << " | score: " << bestClass->total_score() << "\n";
            std::cout << "\n";
            std::cout << "\Worst Performing Class(s)...\n";
            std::cout << "ID: " << worstClass->ID() << " | Lecturer: " << find_lecturer_by_id(worstClass->lecturer_ID())->name() << " | score: "<< worstClass->total_score() << "\n";
            std::cout << "\n";
        }
       
        void display_classes() {
            for (const Class& objClass : objClasses)
            {
                auto lecturer = find_lecturer_by_id(objClass.lecturer_ID());
                auto subject = find_subject_by_id(objClass.lecturer_ID());
                auto department = find_department_by_id(lecturer->departmentID());
                auto students = find_students_in_class(objClass.ID());
               
                std::cout << "Class ID: " << objClass.ID() << "\n";
                std::cout << "Class Size: " << objClass.size() << "\n";
                std::cout << "Lecturer: " << lecturer->name() << "\n";
                std::cout << "Subject: " << subject->name() << "\n";
                std::cout << "Department: " << department->name() << "\n\n";     
            }
        }
  
        bool is_class_id_valid(int class_id) {
            if (auto it = std::find_if(objClasses.begin(), objClasses.end(), [&](auto &objClass) {
                return class_id == objClass.ID();
                }) == objClasses.end()) {
                return false;
            }
            return true;
        }

        bool add_student(std::string name, int classID, int result) {

            if (is_class_id_valid(classID)) {
                objStudents.emplace_back(Student{ name, classID, result });
                find_class_by_id(classID)->total_score(result);
                find_class_by_id(classID)->increment_size();
                return true;
            }
            return false;
        }
    };

int main()
{
    University objUniversity;

    //viewing class 1
    std::cout << "View class students...\n";
    objUniversity.view_class_students(1);

    std::cout << "Class report...\n";
    objUniversity.class_report(1);

    //viewing class 2
    std::cout << "View class students...\n";
    objUniversity.view_class_students(2);

    std::cout << "Class report...\n";
    objUniversity.class_report(2);

    std::cout << "Overall classes report...\n";
    objUniversity.overall_classes_report();
    
    //Run via menu system
    //menu_options(objUniversity);
}

Answer 1

yagni

There doesn't seem to be any requirement in the spec for Subject or Department to be more than simple std::strings.

---

handling ids:

There's a lot of duplicate code for ID handling. We could factor that into a class something like the following (not actually tested):

template<class T>
class ID
{
public:
    
    ID():
        m_ID(newID()) { }
    
    ID(ID const&) = default;
    ID& operator=(ID const&) = default;
    ID(ID&&) = default;
    ID& operator=(ID&&) = default;
    
private:
    
    static int newID()
    {
        static int id = 0;
        return id++;
    }
    
    friend bool operator==(ID a, ID b) { return a.m_ID == b.m_ID; }
    friend bool operator!=(ID a, ID b) { return !(a == b); }
    
    int m_ID;
};

And then use it like:

struct Lecturer
{
    ID<Lecturer> m_ID;
    ...
};

struct Student
{
    ID<Student> m_ID;
    ...
};

With the template argument we get type-safety: we can't pass a lecturer id to a function expecting a student id (and vice-versa) because they are different types. Similarly the operator== and operator!= allow us to compare ids of the same type, but not ids of different type.

Note that we get a separate counter for each type.

We could add an operator>> if desired.

(Note that there's no need for a common base class here. The various types are unrelated, and we have no need to store them together or treat them with the same interface.)

---

fix the specification!

There's a couple of things I'd question about the design:

• Do Students really only take one Class?
• Why is there a Size variable in the Class?

I'd probably remove the Class ID and Result variable from Student, and store a std::vector of Student IDs and their Results in the Class instead.

---

fix bugs:

    auto best_performing_class() {
        return std::max_element(objClasses.begin(), objClasses.end(), [&](const Class& lhs, const Class& rhs)
            {
                return lhs.total_score() < rhs.total_score();
            });
    }

    auto worst_performing_class() {
        return std::min_element(objClasses.begin(), objClasses.end(), [&](const Class& lhs, const Class& rhs)
            {
                return lhs.total_score() < rhs.total_score();
            });
    }

These should use use average score, not total score. (A small class with high scores should be better than a large one with low scores).

---

fix bug, do the simplest thing:

    double average(std::vector<Student>& students) {
        
        auto sum = std::transform_reduce(students.begin(), students.end(), 0.0, std::plus<>(),
            [](auto& student) { return student.result(); });
        return sum / objStudents.size();
    }

I don't think you want objStudents there.

A simple loop would be much clearer:

auto sum = 0;
for (auto const& student : students)
    sum += student.result();

return (double)sum / (double)students.size();

---

explicitly handle edge-cases, use the correct type:

        if (scores.size() % 2 == 0) 
        {
            return (scores.at(scores.size() / 2  - 1) + scores.at(scores.size()/2)) / 2;                        
        }
        return scores.at(scores.size()/2);

What if scores is empty? If it's a precondition for this function that scores isn't empty, we could show that explicitly with an assert(!scores.empty()), or if (scores.empty()) throw std::runtime_error("...");

We should probably return a double here too for a more accurate result.

---

const correctness:

All of the calculation / printing functions should be marked const, since they don't change the class member data (but see also about static functions below).

Any variables passed by reference, where we don't need to change the variable should also be marked const.

e.g.:

double average(const std::vector<Student>& students) const { ...

---

prefer static for helper functions:

Rather than directly accessing class member data in a helper function, it's often clearer to make the functions static. This makes the inputs explicit and obvious, and will help prevent mix ups like objStudents vs students above:

static double average(const std::vector<Student>& students) { ...

Now we can't access class data inside the function - just the specified arguments.

Most of the calculation functions would be better as static functions.

---