Skip to main content
added 245 characters in body
Source Link
user673679
  • 11.9k
  • 2
  • 33
  • 65

(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.)

(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.)

Source Link
user673679
  • 11.9k
  • 2
  • 33
  • 65

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.


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.