yagni
There doesn't seem to be any requirement in the spec for Subject
or Department
to be more than simple std::string
s.
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.