10
\$\begingroup\$

I wrote a simple Cumulative grade-point-average calculator in C++, and would like to ask for advices to improve the code in terms of best practices (efficiency, reliability) in the language. Here's my code:

#include <stdio.h>
#include <iostream>
#include <string>
#include <vector>

class Subject
{
    public:
        std::string name;
        unsigned creditHour, mark;

        Subject(const std::string& name, unsigned creditHour, unsigned mark)
        : name(name), creditHour(creditHour), mark(mark)
        {} 
};

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;
        }
};

class CGPAPrinter
{
    public:
        void printSubjectsSummary(std::vector<Subject*> subjects)
        {
            for(Subject* s : subjects)
            {
                std::cout << "Subject:\t" << s->name << "\t\t";
                std::cout << "Credit Hours:\t" << s->creditHour << "\t\t";
                std::cout << "Mark:\t" << s->mark << std::endl;
            }
        }

        void printCGPA(double cgpa)
        {
            std::cout << "Your CGPA is: " << cgpa << std::endl;
        }
};

int main()
{
    std::string subjectName;
    unsigned creditHour, mark, numOfSubjects;
    double gradePointsSum = 0;
    std::vector<Subject*> subjects;
    CGPACalculator calculator;
    CGPAPrinter printer;

    // Get input from user
    std::cout << "Enter number of subjects: \t";
    std::cin >> numOfSubjects;

    for(int i = 0; i < numOfSubjects; i++)
    {
        std::cout << "Enter subject name: \t";
        std::cin >> subjectName;

        std::cout << "Enter credit hours: \t";
        std::cin >> creditHour;

        std::cout << "Enter mark: \t";
        std::cin >> mark;
        
        gradePointsSum += calculator.getGradePointWith(mark);

        Subject *s = new Subject(subjectName, creditHour, mark);
        subjects.push_back(s);
    }

    printer.printSubjectsSummary(subjects);
    printer.printCGPA(calculator.calculateCGPAWith(gradePointsSum, numOfSubjects));

    return 0;
}

Appreciate your valuable feedbacks.

\$\endgroup\$

3 Answers 3

16
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Why use a vector of pointers? The std::vector also works with local variables, as it would make a copy when using the push_back() method. \$\endgroup\$ Nov 2, 2023 at 20:18
8
\$\begingroup\$

truncated division

            if (mark <= 40 && mark > 30) return 4;

A mathematician would write that as 30 < mark ≤ 40. At least when using a left-to-right writing system, since we conventionally picture the (positive) real number line starting at the left and extending rightwards. In C, prefer to phrase it as

            if (30 < mark && mark <= 40) return 4;

getGradePointWith() is painfully long, suffering from repeated copy-pasta. Truncated integer division can help us out.

The basic function you're computing by piecewise intervals is int gp = 1 + (mark - 1) / 10.


round to N display places

            std::cout << "Your CGPA is: " << cgpa << std::endl;

We probably want printCGPA() to round off that quotient to some customary number of decimal places, perhaps four.

\$\endgroup\$
3
\$\begingroup\$

You haven't defined what the CGPA should be, but this seems to be calculating a simple average, without using the credits allotted to a subject at all. As I understand it, (C)GPA is a weighted average with the credits being the weights, so we should have:

// somewhere above the loop
unsigned totalCredits = 0;

And then:

    // the loop begins
    // ...
    gradePointsSum += creditHour*calculator.getGradePointWith(mark);
    totalCredits += creditHour;

And instead of dividing by the number of subjects, we divide by the total credits:

printer.printCGPA(calculator.calculateCGPAWith(gradePointsSum, totalCredits));

(Then gradePointsSum should be gradePointsWeightedSum or something like that.)

\$\endgroup\$

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.