C vs C++ headers
#include <stdio.h>
This is a C header. For C++ we'd want #include <cstdio> instead (which puts the contents in the std:: namespace). Though it looks like we're not using anything from this header anyway.
Aggregate initialization
class Subject { public: std::string name; unsigned creditHour, mark; Subject(const std::string &name, unsigned creditHour, unsigned mark) : name(name), creditHour(creditHour), mark(mark) { } };
In modern C++ we don't need to add a constructor just to initialize member variables like this. We can write this as a simple struct, and use aggregate initialization:
struct Subject { std::string name; unsigned creditHour, mark; }; ... Subject *s = new Subject{ subjectName, creditHour, mark };
Classes vs free functions
class CGPACalculator { public: unsigned getGradePointWith(unsigned mark) { if (mark <= 100 && mark > 90) return 10; if (mark <= 90 && mark > 80) return 9; if (mark <= 80 && mark > 70) return 8; if (mark <= 70 && mark > 60) return 7; if (mark <= 60 && mark > 50) return 6; if (mark <= 50 && mark > 40) return 5; if (mark <= 40 && mark > 30) return 4; return 0; } double calculateCGPAWith(double gradePointsSum, unsigned numOfSubjects) { return gradePointsSum / numOfSubjects; } };
This class has no state (i.e. contains no member variables), so it should not be a class. This also applies to the CGPAPrinter class. We can use a namespace to group related functions instead of a class:
namespace GPA { struct Subject { std::string name; unsigned creditHour, mark; }; unsigned getGradePointWith(unsigned mark) { ... } double calculateCGPAWith(double gradePointsSum, unsigned numOfSubjects) { ... } void printSubjectsSummary(std::vector<Subject *> subjects) { ... } void printCGPA(double cgpa) { ... } } // GPA
Now we don't need to create any unnecessary objects. We can just call the functions we need.
Variable declaration
int main() { std::string subjectName; unsigned creditHour, mark, numOfSubjects; double gradePointsSum = 0; std::vector<Subject *> subjects;
Declaring all variables at the top of a scope is an obsolete and unnecessary habit from ancient C. We should instead declare variables where they are first needed, initialize them to useful values, and avoid reusing them where practical. Something like:
int main() { std::cout << "Enter number of subjects: \t"; unsigned numOfSubjects; std::cin >> numOfSubjects; std::vector<Subject *> subjects; double gradePointsSum = 0.0; for (unsigned i = 0; i < numOfSubjects; i++) { std::cout << "Enter subject name: \t"; std::string subjectName; std::cin >> subjectName; std::cout << "Enter credit hours: \t"; unsigned creditHour; std::cin >> creditHour; std::cout << "Enter mark: \t"; unsigned mark; std::cin >> mark; gradePointsSum += GPA::getGradePointWith(mark); Subject *s = new Subject{ subjectName, creditHour, mark }; subjects.push_back(s); } GPA::printSubjectsSummary(subjects); GPA::printCGPA(GPA::calculateCGPAWith(gradePointsSum, numOfSubjects)); return 0; }
Memory management
std::vector<Subject *> subjects; ... Subject *s = new Subject{ subjectName, creditHour, mark };
If we use new to allocate memory we must ensure we clean it up again by calling delete (to avoid leaking memory). We should generally avoid manual memory management and use a smart pointer (e.g. std::unique_ptr) instead.
However, in this case it's unnecessary. We can simply store the subjects by value:
std::vector<Subject> subjects; ... subjects.emplace_back(subjectName, creditHour, mark);
User input
std::cout << "Enter number of subjects: \t"; unsigned numOfSubjects; std::cin >> numOfSubjects;
We should add some error checking to ensure that the user actually entered a number, and that we successfully read it.
Aside
Contrary to the other answer, I actually don't mind the series of if statements in:
unsigned getGradePointWith(unsigned mark) { if (mark <= 100 && mark > 90) return 10; if (mark <= 90 && mark > 80) return 9; if (mark <= 80 && mark > 70) return 8; if (mark <= 70 && mark > 60) return 7; if (mark <= 60 && mark > 50) return 6; if (mark <= 50 && mark > 40) return 5; if (mark <= 40 && mark > 30) return 4; return 0; }
But, we could make it simpler by only checking one bound (since we're checking in descending order):
unsigned getGradePointWith(unsigned mark) { if (mark > 100) return 0; // todo: output an error!?? if (mark >= 91) return 10; if (mark >= 81) return 9; if (mark >= 71) return 8; if (mark >= 61) return 7; if (mark >= 51) return 6; if (mark >= 41) return 5; if (mark >= 31) return 4; return 0; }
It's much easier to glance at this and understand the grade boundaries than it is to understand the nuances of a math equation.