I'm trying to develop an OOP way of thinking. I was hoping someone would be kind enough to spare some of their valuable time to review my Grade Calculator OOP program. As always, I'd like to know what I've done well, what I should improve upon, and any suggestions on how I could perhaps improve what I have? By the way, I have a class called Class. I probably should prefix it with "cls" so as not to confuse. Treat this program as is supposed to be entered, I haven't error checked it. The point of this program is to develop in OOP.

   // Task 1.cpp : This file contains the 'main' function. Program execution begins and ends there.

#include <iostream>
#include <vector>
#include <algorithm>
#include <functional>
#include <numeric>
#include <string>
class TestPaper
    int m_scoreOutOf;  
    bool checkBoundary(int value, int boundary) {
        if (value < 0 || value > boundary) {
            std::cout << "Score must be between " << " 0 and " << boundary << ". Please try again.\n";
            return false;
        return true;

class Student {
    std::string m_name;
    int m_scoreGot;
    Student(std::string name, int scoreGot)
        :m_name(name), m_scoreGot(scoreGot){}

    std::string getName() const { return m_name; }
    int getScoreGot() const { return m_scoreGot; }

class Class {
    void AddStudent(TestPaper& testPaper) {
        std::string name = "";
        int scoreGot = 0;
        std::cout << "Enter student name: ";
        std::getline(std::cin >> std::ws, name);
            std::cout << "\nWhat did " << name << " score?\nEnter a score between 0 and "
                << testPaper.m_scoreOutOf << ": ";
            std::cin >> scoreGot;
        } while (testPaper.checkBoundary(scoreGot, testPaper.m_scoreOutOf) == false);
        students.push_back({ name, scoreGot });

    std::vector<Student>& accessStudents() { return students; }

class GradeCalculator {
    TestPaper m_testPaper;
    Class m_ClassOfStudents;
    GradeCalculator(TestPaper testPaper, Class classOfStudents) :m_testPaper(testPaper), m_ClassOfStudents(classOfStudents) {}
    void DisplayMenu() {

        std::cout << "\n1. Add student and their grade\n";
        std::cout << "2. Calculate class score\n";
        std::cout << "3. Modify testpaper (haven't implemented this yet)\n";

    double averageGrade() {
        auto sum = std::transform_reduce(m_ClassOfStudents.accessStudents().begin(), m_ClassOfStudents.accessStudents().end(), 0.0, std::plus<>(),
            [&](auto& student) { return calculateGradePercentage(student); });
        return sum / m_ClassOfStudents.accessStudents().size();
    double calculateGradePercentage(Student &student)
        return static_cast<double>(student.getScoreGot()) / static_cast<double>(m_testPaper.m_scoreOutOf) * 100;
    void DisplayResult() {
        for (auto& student : m_ClassOfStudents.accessStudents()) {
            std::cout << "Percentage scores are: \n";
            std::cout << student.getName() << ": " << calculateGradePercentage(student) << "%\n";
        std::cout << "Average grade perecentage: " << averageGrade() << "%\n";
    void runProgram() {

        int menuChoice = 0;
        while (true)
            std::cout << "\nEnter a choice from the menu: ";
            std::cin >> menuChoice;
            switch (menuChoice)
            case 1:
            case 2:
                std::cout << "Invalid choice!\n";

int main()
    TestPaper testPaper({ 20 });
    Class classOfStudents;
    GradeCalculator calculator(testPaper, classOfStudents);

1 Answer 1


Here are some things to help you improve your program.

Don't mix I/O with data operations

Generally speaking having a std::getline and std::cout within a data class such as Class is not a good idea. It makes it harder to reuse the class. Better practice is to keep the data (Student, Class, etc.) separate from getting input from users. The Model-View-Controller design pattern is often useful for programs like this. Consider using something like a ConsoleMenu class as in this answer.

Use move semantics where practical

In the AddStudent function, we have this line:

students.push_back({ name, scoreGot });

Better would be to use emplace_back which tells the compiler that it doesn't need to construct and copy, but that it's safe to construct the object in place.

Rethink the class interface

The Class class has this member function:

std::vector<Student>& accessStudents() { return students; }

This is a bad idea. It returns a reference to an internal class member. Think what happens if the Class instance is deleted but some external entity is still holding a reference to data that no longer exists. The only place it's used is within GradeCalculator::averageGrade() and GradeCalculator::DislayResult() so that's a strong indicator that something is not right in the class interface. I'd suggest making the averageGrade() function a Class member function.

Use defaults when sensible

The Student constructor is essentially the same as that which would have been generated by the compiler. To reduce the chance of error, it can be simply eliminated.

Think about the user

There is no obvious way to gracefully exit the program. I'd suggest adding a menu item for that. Also, chances are that anyone actually using the program would want to enter the entire class of students and then calculate the scores for the whole class. Having to repeatedly select "1. Add student..." is a bit tedious. Better might be to have the program gather input until an empty name or maybe "quit" was entered, and then automatically show the scores. Better still would be to allow input from a text file.

  • \$\begingroup\$ Thank you for your instructive feedback! I am going to reflect on everything you have said. I may have questions later on! \$\endgroup\$ Dec 19, 2020 at 7:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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