11
\$\begingroup\$

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; 
    int m_iGrade;
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"
    int GetGrade() const; //Allows for external access to the private member variable "m_sName"
    std::string CalculateGradeLetter() const; //returns the "letter" of the grade
    std::string HasUserPassed(); //Determines whether the user has passed by checking if the grade returned is "F"
};

//Implementations for the "Student" class
Student::Student(std::string sName, int iGrade) :m_sName(sName), m_iGrade(iGrade) {}
std::string Student::GetName() const {
    return m_sName;
}

int Student::GetGrade() const {
    return m_iGrade;
}

std::string Student::CalculateGradeLetter() const
{
    if (m_iGrade < 10) {
        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";
    }
    else if (m_iGrade < 100)
    {
        return "A*";
    }
    else
    {
        return "Invalid";
    }
}

std::string Student::HasUserPassed()
{
    std::string Grade = CalculateGradeLetter();
    if (Grade == "F")
    {
        return "Fail";
    }
    return "Passed";
}
//End of interface link 


//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 HighestGradeIndex(std::vector<Student>& objStudents) {
    std::size_t stIndex = 0;
    int iHighestGrade = 0;
    for (std::size_t st = 0; st < objStudents.size(); st++)
    {
        if (objStudents.at(st).GetGrade() > iHighestGrade)
        {
            iHighestGrade = objStudents.at(st).GetGrade();
            stIndex = st;
        }
    }
    return stIndex;
}
//returns the index of the vector of the student who got the lowest grade
std::size_t LowestGradeIndex(std::vector<Student>& objStudents)
{
    std::size_t stIndex = 0;
    int iLowestGrade = INT_MAX;
    for (std::size_t st = 0; st < objStudents.size(); st++) {
        if (objStudents.at(st).GetGrade() < iLowestGrade)
        {
            iLowestGrade = objStudents.at(st).GetGrade();
            stIndex = st;
        }
    }
    return stIndex;
}

//Through its parameters the average value is passed in to subsequently check its value with the if / else logic
std::string CalculateGradeLetter(double dAverage)
{
    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) {

    int iHighestGrade = objStudents.at(HighestGradeIndex(objStudents)).GetGrade();
    int iLowestGrade = objStudents.at(LowestGradeIndex(objStudents)).GetGrade();
    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
int AddGrade(std::vector<Student>& objStudents) {

    int AddedGrade = 0;
    for (Student& objStudent : objStudents) {
        AddedGrade += objStudent.GetGrade();
    }
    return AddedGrade;
}
//Code self explanatory
double AverageGrade(std::vector<Student>& objStudents)
{
    return (double)AddGrade(objStudents) / objStudents.size();
}
//Code self explanatory
void DisplayGradeResult(std::vector<Student>& objStudents) {
    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 << "Grade: " << Student.CalculateGradeLetter() << std::endl;
    }
    std::cout << std::endl;
}

void DisplayAverage(std::vector<Student>& objStudents)
{
    double dAverageGrade = AverageGrade(objStudents);
    std::cout << "Overall average grade is " << AverageGrade(objStudents) << " which is " << CalculateGradeLetter(dAverageGrade) << std::endl;
}

void AddStudent(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;
    //AddStudent(objStudents); //Manually add users here 
    TestData(objStudents); //Load test users without manually writing them
    DisplayGradeResult(objStudents);
    WhoGotTheHighest(objStudents);
    DisplayAverage(objStudents);
}
\$\endgroup\$
0
6
\$\begingroup\$

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) {
    return a.GetGrade() < b.GetGrade();
});

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){
        return a.GetGrade() < b.GetGrade();
    });

    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();
}
\$\endgroup\$
5
  • \$\begingroup\$ THANK YOU! I appreicate this greatly. I will have questions. I'm currently just trying to understand and implement all suggestions given to me. \$\endgroup\$ Oct 21 '20 at 22:13
  • \$\begingroup\$ that std::transform_reduce is giving me errors despite including the algorithm library. It doesn't recongise it from the std:: library. Solutions? \$\endgroup\$ Oct 23 '20 at 13:47
  • 2
    \$\begingroup\$ It requires C++17. I added an example using std::accumulate() which you can use with older C++ versions. \$\endgroup\$
    – G. Sliepen
    Oct 23 '20 at 15:40
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ Oct 23 '20 at 22:09
  • \$\begingroup\$ 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. \$\endgroup\$ Oct 23 '20 at 22:57
13
\$\begingroup\$

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)
    {
        int highestGrade = 0;
        for (const Student& s : students)
        {
            if (s.getGrade() > highestGrade)
            {
                highestGrade = s.getGrade();
            }
        }
    }
  • 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
\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – minnmass
    Oct 22 '20 at 20:35
  • \$\begingroup\$ 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. \$\endgroup\$ Oct 23 '20 at 22:59
  • \$\begingroup\$ 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". \$\endgroup\$ Oct 26 '20 at 2:22
10
\$\begingroup\$
  • 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;
      };
    
      grade grades[] {
          { 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)
      {
          for (auto grade: grades) {
              if (score <= grade.cutoff) {
                  return grade.letter;
              }
          }
      }
    

    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.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks so much for reviewing my program and providing excellent suggestions! \$\endgroup\$ Oct 21 '20 at 20:56
  • \$\begingroup\$ This is a great technique to learn and understand. Checkout "Lookup Tables" as a means to map one value to another \$\endgroup\$
    – Dancrumb
    Oct 22 '20 at 14:43
  • 1
    \$\begingroup\$ Note, you reinvented std::lower_bound \$\endgroup\$
    – YSC
    Oct 23 '20 at 11:13
  • 1
    \$\begingroup\$ @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. \$\endgroup\$
    – vnp
    Oct 23 '20 at 15:22
5
\$\begingroup\$

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.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ "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. \$\endgroup\$
    – alephzero
    Oct 23 '20 at 21:10
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Oct 23 '20 at 21:51
5
\$\begingroup\$

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)
\$\endgroup\$
1
  • \$\begingroup\$ Thanks! This is very instructive! \$\endgroup\$ Oct 23 '20 at 21:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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