I was wondering if you'd be able to review my basic C++ Grade Calculator I based off the following scenario. I would like to know what I've done well vs what I haven't done well. Many thanks in advance.

Program Requirements The client has asked you to create a program that can calculate the average grade for 8 students. The client wants to be able to add 8 names and 8 grades. The student must then be assigned whether they have passed (must be C+) the test and the grade that they have achieved. The client also wants the following reporting to be added to the system; who achieved the largest mark, and the average of marks (which should also be assigned a grade).

Marking Criteria All tests are marked out of 100, with the following grade boundaries Applying to those grades: F: 0-10 , E: 11-30 , D: 31-50 , C: 51-70 , B: 71-80 , A: 81-90 , A*: 90-100

Input Name Marks out of 100

Output Name Has the student passed? What grade did they achieve? What was the highest grade achieved and by who? What was the average mark and grade achieved?

#include <iostream>
#include <vector>
//My own user-defined datatype "Student"
class Student
{
private: //Everything under "private" is only accessible from the class itself and not outside e.g. in the int main()
std::string m_sName;
public:
Student(std::string sName, int iGrade); //Constructor to initialize member variables (in the private)
std::string GetName() const; //Allows for external access to the private member variable "m_sName"
std::string HasUserPassed(); //Determines whether the user has passed by checking if the grade returned is "F"
};

//Implementations for the "Student" class
std::string Student::GetName() const {
return m_sName;
}

}

{
return "F";
}
else if (m_iGrade < 30) {
return "E";
}
else if (m_iGrade < 50) {
return "D";
}
else if (m_iGrade < 70) {

return "C";
}
else if (m_iGrade < 80) {
return "B";
}
else if (m_iGrade < 90) {
return "A";
}
{
return "A*";
}
else
{
return "Invalid";
}
}

std::string Student::HasUserPassed()
{
{
return "Fail";
}
return "Passed";
}

//Stand alone functions (nothing to do with the Student class itself)
//returns the index of the vector of the student who got the highest grade
std::size_t stIndex = 0;
for (std::size_t st = 0; st < objStudents.size(); st++)
{
{
stIndex = st;
}
}
return stIndex;
}
//returns the index of the vector of the student who got the lowest grade
{
std::size_t stIndex = 0;
for (std::size_t st = 0; st < objStudents.size(); st++) {
{
stIndex = st;
}
}
return stIndex;
}

//Through its parameters the average value is passed in to subsequently check its value with the if / else logic
{
if (dAverage < 10) {
return "F";
}
else if (dAverage < 30) {
return "E";
}
else if (dAverage < 50) {
return "D";
}
else if (dAverage < 70) {

return "C";
}
else if (dAverage < 80) {
return "B";
}
else if (dAverage < 90) {
return "A";
}
else if (dAverage < 100)
{
return "A*";
}
else
{
return "Invalid";
}
}

//Code self explanatory
void WhoGotTheHighest(std::vector<Student>& objStudents) {

std::cout << "The person with the highest score is " << objStudents.at(HighestGradeIndex(objStudents)).GetName() << " who got " << iHighestGrade << std::endl;
std::cout << "The person with the lowest score is " << objStudents.at(LowestGradeIndex(objStudents)).GetName() << " who got " << iLowestGrade << std::endl;
}

//Code self explanatory

for (Student& objStudent : objStudents) {
}
}
//Code self explanatory
{
}
//Code self explanatory
for (Student& Student : objStudents)
{
std::cout << "****" << std::endl;
std::cout << "Student: " << Student.GetName() << std::endl;
std::cout << "Has Student passed?: " << Student.HasUserPassed() << std::endl;
}
std::cout << std::endl;
}

void DisplayAverage(std::vector<Student>& objStudents)
{
}

{
std::string sName = "";
int iAge = 0;
std::cout << "Enter student's name: ";
std::cin >> sName;
std::cout << "Enter student's grade: ";
std::cin >> iAge;
objStudents.push_back({ sName, iAge });
}

void TestData(std::vector<Student>& objStudents) {
objStudents.push_back(Student("George", 1));
objStudents.push_back(Student("Jack", 10));
objStudents.push_back(Student("Josh", 40));
objStudents.push_back(Student("Oli", 60));
objStudents.push_back(Student("Josh", 80));
objStudents.push_back(Student("Stephen", 100));
}

int main()
{
std::vector<Student>objStudents;
TestData(objStudents); //Load test users without manually writing them
WhoGotTheHighest(objStudents);
DisplayAverage(objStudents);
}


In addition to what has already been said in the other answers, consider using the algorithms from the standard library to reduce the amount of code you need to write, and perhaps even to avoid the need for some of the functions you wrote entirely.

For example, instead of using HighestGradeIndex(), you can use std::max_element():

auto highest_grade_it = std::max_element(students.begin(), students.end(), [](auto &a, auto &b) {
});


There is also std::min_element() for finding the element with the smallest value, and std::minmax_element() to get both the minimum and maximum in one go. For example:

void WhoGotTheHighest(std::vector<Student>& objStudents) {
auto [lowest, highest] = std::minmax_element(objStudents.begin(), objStudents.end(), [](auto &a, auto &b){
});

std::cout << "The person with the highest score is " << highest->GetName()
<< " who got " << highest->GetGrade() << "\n";
<< "The person with the lowest score is " << lowest->GetName()
<< " who got " << lowest->GetGrade() << "\n";
}


There is also std::transform_reduce() that you can use to compute the sum of the grades of all students, like so:

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


The above requries C++17 support. If you are using an older C++ standard, you can use std::accumulate():

double AverageGrade(std::vector<Student>& objStudents)
{
auto sum = std::accumulate(objStudents.begin(), objStudents.end(), 0.0, [](auto &accum, auto &student){ return accum + student.getGrade(); });
return sum / objStudents.size();
}

• THANK YOU! I appreicate this greatly. I will have questions. I'm currently just trying to understand and implement all suggestions given to me. Oct 21, 2020 at 22:13
• that std::transform_reduce is giving me errors despite including the algorithm library. It doesn't recongise it from the std:: library. Solutions? Oct 23, 2020 at 13:47
• It requires C++17. I added an example using std::accumulate() which you can use with older C++ versions. Oct 23, 2020 at 15:40
• I’ve marked this one as best answer because it has given me an introduction to the “Algorithm library”. I have just watched a YouTube video on it. This has really helped broaden my horizon in terms of tools available which save you from having to reinvent the wheel (so to speak). Thanks very much for pointing me in the right direction and showing me what’s available in this language. I’ve discovered this language is highly sophisticated and continues to surprise me. Truly fascinated by the C++ language. Oct 23, 2020 at 22:09
• The IDE I'm using is Visual Studio 2019. I have gone to Project > Settings > C/C++ > C++ Language Standard and changed it to "ISO C++17 Standard (/std:c++17)" however the std::accumulate() isn't working. How do I resolve this issue? And I have the #include <algorithm> library at the top. It's this sort of thing which prevents me from using these libraries, they simply don't work. Oct 23, 2020 at 22:57

Overall, your code looks pretty reasonable. However, there's a few issues that I can see that could be improved.

Naming Conventions and types

• Having names like sName and iGrade are clunky. Remove the s and i, they're just cluttering your code with redundant information.
• Class names should be PascalCase and function names should be camelCase. That way, it's easier to differentiate between user defined types and functions.
• Consider changing iGrade to something like marks or totalPoints to distinguish how many points were gotten from the actual letter grade.
• Consider changing hasUserPassed from a string to a bool. Then, use that check to output passed or failed. Any time a function should return a true or false, it should be boolean.
• Pass by const &, not just &. That way, you cannot accidentally modify your data.

Logic Issues

• From what I'm seeing, your grades are actually F: 0-9 , E: 10-29 , D: 30-49... and a score of 100 is invalid. This is because your checks are m_iGrade < 10 instead of m_iGrade <= 10 or m_iGrade < 11.
• You have two calculateLetterGrade functions that do exactly the same thing. This looks like you should just have one function that does that work. In your case, it should probably be in Student because you aren't using it outside of Student.
• Rather than returning an index from HighestGradeIndex, consider returning a student. Then you do not have to use the vector again to output the information and it will clean up your syntax a little bit.

General C++ Things

• For each loops are amazing when you know you are looping through all elements of a container. The logic of your highestGrade (and then lowestGrade) would then look something like this:
    findHighestScoringStudent(const std::vector<Student>& students)
{
for (const Student& s : students)
{
{
}
}
}

• If you return a student from highest and lowest, you don't need the whoGotHighestandLowest function.
• AverageGrade and AddGrade can just be onw function, just combine their logic together.
• Don't use (double) to cast, prefer using static_cast<double>(variable). static_cast is checked by the compiler at compile time instead of run time so you can avoid problems there.
• If you want to do more c++11 things, consider using auto. It automatically deduces the type of the variable based on the right side of the = operator. Like so const auto& s : Students can be used to loop through students because it's easy to see that s is of type Student
• I suspect that sName and iGrade are attempts at Hungarian notation. If so, this blog post has (IMO) a great take on how Hungarian should be (and was intended to be) used. TL:DR: use prefixes to denote "kind", not "type" (eg., ix for an array index and d for the difference between numbers - so dx would be a width - rather than denoting that the variable is a string/int/...) - the prefix should help a reader see that something is off, not remind them what the data's type is. Oct 22, 2020 at 20:35
• It's something I'm required to do. My teacher said we have to use this convention. I can't argue and he's the one who marks my work. Oct 23, 2020 at 22:59
• There's another logic error you missed. The requirements say that a grade of C+ (I assume that means C or better) is passing, but the HasUserPassed function only considers an F to be a failing grade. Also, I don't know why it's called HasUserPassed instead of HasStudentPassed. You also have a typo: "onw function". Oct 26, 2020 at 2:22
• The score validation belongs to the constructor. You want to catch errors as soon as possible. BTW, your code happily accepts negative scores.

• Separate data from logic. Consider

  struct grade {
int cutoff;
std::string letter;
};

{ 10, "F" },
{ 30, "E" },
{ 50, "D" },
{ 70, "C" },
{ 80, "B" },
{ 90, "A" },
{ 100, "A*" },
};


Then the letter assignment becomes

  std::string compute_grage_letter(int score)
{
}
}
}


This looks neater then the wall of if/elses, and has an additional benefit: now you may load the score/letter relations from the file.

• I don't think that grade letter calculation belongs to Student. It has nothing to do with the particular student, and rather implements the school policy.

• Prefer [] to at(). The latter does bound checking. If you are sure that all accesses are in bound (and you better should be!), bound checking is a waste of time.

• I don't know how far you are at C++; just FYI, STL provides std::min and std::max.

• AddStudent inputs the grade into iAge. Very strange.

• Thanks so much for reviewing my program and providing excellent suggestions! Oct 21, 2020 at 20:56
• This is a great technique to learn and understand. Checkout "Lookup Tables" as a means to map one value to another Oct 22, 2020 at 14:43
• Note, you reinvented std::lower_bound
– YSC
Oct 23, 2020 at 11:13
• @YSC No. std::lower_bound is a binary search. I intentionally presented the linear version, because as far as I can tell OP is not supposed to know about binary yet.
– vnp
Oct 23, 2020 at 15:22

One other thing:

The instructions say that a passing grade is "C+" (presumably "C" or higher), so the implementation of HasUserPassed() is incorrect.

It's probably simpler to compare the mark as a number, instead of fetching the grade letter and comparing that.

• "Comparing the mark as a number" is a poor design. You already have two existing functions: convert a number to a grade, and convert a grade (NB a grade, not a number!) to pass/fail. Either of these could be changed independently (e.g. somebody decides that the grade boundary marks should be changed, or that grade D now counts as a pass). You do NOT want to write a third "convert mark into pass/fail" function which performs the same tasks a different way. That will be harder to maintain - one day, somebody will forget that you have to change it when you change one of the other functions. Oct 23, 2020 at 21:10
• A very valid point there. I didn’t think of the maintainability aspect! I will think of a way to resolve that issue. Many thanks! This forum has certainly given a lot of food for thought and has definitely taught me many things. Oct 23, 2020 at 21:51

Here's some advice that might get you into trouble if this is homework: don't write so many comments. Some teachers demand comments on everything, but this is a bad idea in professional practice. Every time there's redundancy built into data, there's a chance to develop inconsistencies; likewise, it's easy for what the code actually does and what the comments say the code does to drift apart. In general, code should be as self-documenting as possible; comments should only be added when they add something that can't be determined by reading the code itself.

In general, you should use comments for documentation, not explanation. Don't write what code does: write why it does it, or why it has to do it that particular way.

One more thing: by convention, the audience of your comments depends on where it's placed. Above a function body or prototype, you're addressing a user of the function; inside a function body, you're talking to a maintainer of the function's code. Respect the principle of encapsulation by not talking about implementation-dependent details in comments outside of the implementation itself. For example, don't mention that you're using an if/else ladder as far as the user of your function should care, you could be using a switch statement. In the same manner, don't tell the user of an accessor function that it returns the contents of a member variable: the whole point of accessors is that whoever uses them doesn't have to know or care whether they're actually touching a member variable, or if the value is calculated on the fly from something else, or if it's stored in some sort of global hash table or backed by a database query or who knows what.

A few more examples referring to specific comments in this piece of code (not exhaustive, and I'm sure you get the general idea):

• Don't explain that "class Student" creates a class called Student (of course it does)
• Don't explain that your private members aren't accessible outside the class's methods (of course; that's literally what theprivate keyword does)
• Don't explain that your constructor initializes member variables (of course; that's what constructors do)
• Don't label code as self-explanatory (that's literally what not writing a comment means)
• Thanks! This is very instructive! Oct 23, 2020 at 21:53