8
\$\begingroup\$

I've started learning C++, coming from a PHP/JS background. I've got a copy of the Accelerated C++ book and I'm working through it, doing each chapter and exercise. Having done a few, I'd like to get some feedback to make sure I'm not doing anything too wrong.

The chapter is introduced with the premise of

Imagine a course in which each student's final exam counts for 40% of the final grade, the midterm exam counts for 20%, and the average homework grade makes up the remaining 40%.

The particular exercise I'm doing then says

Write a program that will keep track of grades for several students at once. The program could keep two vectors in sync: the first should hold the students' names and the second the final grades that can be computed as input is read. For now, you should assume a fixed number of homework grades.

They suggest using two vectors for this; one of student names and one of grades, although I expanded slightly on that to take into account the different grades (midterms, finals and homework). I've also opted to calculate the grades after receiving all input in order to separate the code that gets and stores input from the code that performs calculations.

Here's a full copy of the program as it stands (110 lines). I'm looking for any feedback on style, correctness, language traps I may have accidentally fallen into, and really anything in general that you think may be of use.

#include <iostream>
#include <iomanip>
#include <string>
#include <vector>

using std::cin;
using std::cout;
using std::endl;
using std::setprecision;
using std::streamsize;
using std::string;
using std::vector;

/*****
 * This program is designed to take a list of students, then ask for their
 * midterm, final and homework grades. It then calculates the final grade
 * for each student, with the following weights:
 *   Midterm:  20%
 *   Final:    40%
 *   Homework: 40% of average grade
 */
int main()
{
  // How many homework grades each student requires
  int assignments = 3;

  // Get all student names and store them in the first vector
  vector<string> students;
  {
    string student;
    cout << "Please enter all students forenames, or an empty line when done."
         << endl
         << "Student name: ";

    while (getline(cin, student) && !student.empty()) {
      students.push_back(student);
      cout << "Student name: ";
    }

    cout << endl
         << students.size() << " students entered."
         << endl << endl;
  }

  // Ask for the midterm, final and `assignments` homework grades for students
  vector<double> midterms;
  vector<double> finals;
  vector<double> homework;
  {
    double grade;

    for (int s = 0; s < students.size(); s++) {
      // invariant: we've received grades for `s` students
      cout << "Grades for " << students[s] << endl;

      cout << " - Midterm: ";
      cin >> grade;
      midterms.push_back(grade);

      cout << " - Final: ";
      cin >> grade;
      finals.push_back(grade);

      for (int a = 0; a < assignments; a++) {
        // invariant: we've received the grade for `a` assignments
        cout << " - Assignment " << a + 1 << ": ";
        cin >> grade;
        homework.push_back(grade);
      }
      cout << endl;
    }
  }

  // Calculate and print the overall grades
  cout << "Overall Grades:" << endl;
  streamsize precision = cout.precision();
  {
    typedef vector<string>::size_type vec_size;
    double overallGrade,
           midtermGrade,
           finalGrade,
           homeworkGrades;

    for (vec_size s = 0; s < students.size(); s++) {
      midtermGrade   = midterms[s];
      finalGrade     = finals[s];
      homeworkGrades = 0;

      // Pull out the homework grades
      int start = s * assignments,
          end   = (s + 1) * assignments - 1;

      for (vec_size h = 0; h < assignments; h++) {
        // invariant: we've summed the grades for
        homeworkGrades += homework[start + h];
      }

      // Calculate the overall grade
      overallGrade = 0.2 * midtermGrade
                     + 0.4 * finalGrade
                     + 0.4 * homeworkGrades / assignments;

      cout << " - " << students[s] << ": "
           << setprecision(3) << overallGrade << setprecision(precision)
           << endl;
    }
  }

  return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for not using using namespace std; \$\endgroup\$ – yuri Jun 6 '17 at 13:34
  • \$\begingroup\$ @yuri I'd like to say that's intentional, but it's simply the way the book introduces include and using statements, so I don't know about namespace std; :P I'll look it up for reference of what not to do, though :-) \$\endgroup\$ – Joe Jun 6 '17 at 13:40
  • \$\begingroup\$ At this point I'd refactor the code into functions. You're not going to write real-world, C++ applications in one function, right? (Although I commend your use of braces to limit scope of variables!) \$\endgroup\$ – RichN Jun 6 '17 at 14:03
  • \$\begingroup\$ @RichN Comments are for seeking clarification to the question, and may be deleted. Please write all suggestions as answers. \$\endgroup\$ – 200_success Jun 6 '17 at 14:37
  • 1
    \$\begingroup\$ You can look it up here :-) \$\endgroup\$ – Cody Gray Jun 7 '17 at 0:46
6
\$\begingroup\$

Overall, not bad for a beginner in my opinion. Some things to note:

  • Use compiler warnings

    There are several (minor) warnings that you probably missed. One of them about an unused variable.

  • endl vs \n

    I'm sure your book mentions this but endl will flush the buffer while \n won't.
    In most cases using \n suffices.

  • Group logic together

    cout << "Student name: "; This is repeated and could be eliminated if you restructure the loop slightly.

    You should also declare variables as late as possible. So move the declaration of student right in front of the loop.

    string student;
    for (;;)
    {
        cout << Student name: ";
        getline(cin, student);
        if (student.empty())
        {
            break;
        }
        students.push_back(student);
    }
    
  • Use range-based for loops

    You can use the new range based for loops instead of the old style loops e.g. for (const auto &s : students) { instead of for (int s = 0; s < students.size(); s++) {

    Adjust the next line accordingly: cout << "Grades for " << s << endl;

  • Make constants constant

    The assignments variable is never changed and should therefore be declared as a constant.

  • Use functions

    You are already scoping the code which is good but as @RichN pointed out you might as well use functions instead.

  • Prefer using over typedefs

    As Scott Meyers suggests in his book Effective Modern C++, you should prefer the new using directive over typedefs. For example:

    using vec_size = vector<string>::size_type;
    
\$\endgroup\$
  • \$\begingroup\$ Thanks, that's really useful. I've got a couple of additional questions on the back of this - 1) Which compiler flags should I use to enable the appropriate level of warnings? 2) What would be the syntax to convert my typedef to using? I tried a very naive using vector<double>::size_type; but that doesn't compile \$\endgroup\$ – Joe Jun 6 '17 at 16:15
  • 2
    \$\begingroup\$ If you're using gcc, you could use gcc -Wall -Wextra -pedantic which is what I use by default. For the using, it would look like this: using vec_size = vector<string>::size_type; \$\endgroup\$ – Edward Jun 6 '17 at 16:26
  • 2
    \$\begingroup\$ @Joe To make all warnings fatal errors (as they should be with C and C++ compilers) you can add to that list -Werror -pedantic-errors \$\endgroup\$ – cat Jun 6 '17 at 19:17
  • \$\begingroup\$ Makes sense. With the range-based for loop, what's the difference between const auto& s and const auto &s (if there is one, and if it makes sense). From what I can see, the & indicates the constant is a reference, but in the context of a loop I'm not sure how that works. I'd normally encounter references (in PHP for example) as $a = (object) [ 'foo' => 'bar' ]; $b = $a; - $b is now pointing to the same address as $a, and if one changes, both will. With a loop counter, how does that work? Apologies if this comment is nonsensical, I haven't properly got to references yet :P \$\endgroup\$ – Joe Jun 7 '17 at 7:52
  • \$\begingroup\$ @Joe No difference, just be consistent. Here is some more info about the loop. \$\endgroup\$ – yuri Jun 7 '17 at 8:32
5
\$\begingroup\$

I see some things that may help you improve your code. I should note that although you're a beginner, I'm going to review the code and offer suggests as though you were expert in C++. It's not to be pedantic, but rather to try to show how C++ code might look in the future when you've acquired more knowledge and experience. Don't worry if you haven't seen all of these things yet. They will come in time.

Break up the code into smaller functions

The main() function is quite long and does a series of identifiable steps. Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function.

Eliminate "magic numbers"

This code contains some "magic numbers," that is, unnamed constants such as 0.2, 0.4, 3, etc. Generally it's better to avoid that and give such constants meaningful names. That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "0.4" and then trying to determine if this particular 0.4 means the weighting of the final grade or some other constant that happens to have the same value.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream. Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.

Don't store data you don't need

The description you quoted contains a hint to this effect when it says:

the final grades that can be computed as input is read.

There's no inherent problem with your approach, and it might actually be useful if it were required to be able to print each of the grades individually. If it's not required, the calculation can be done as each grade is read and only the final grade need be retained.

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of the game (which does not depend on the underlying OS).

Consider using a custom object

If we consider that each student record contains a name and a final grade, we might create an object:

class StudentRecord {
    std::string name;
    double grade;
};

We can then have a single vector containing all records:

std::vector<StudentRecord> grades;

Printing each record could be done via a friend function:

friend std::ostream& operator<<(std::ostream &out, const StudentRecord &sr) {
    return out << sr.name << ': ' << std::setprecision(3) << sr.grade;
}

Using these plus an inserter function (not shown), your main could look like this:

int main() {
    std::vector<StudentRecord> grades;
    for (StudentRecord sr; std::cin >> sr; grades.push_back(sr)) 
    {}
    for (const auto &sr : grades) {
        std::cout << sr << '\n';
    }
}

Minimize the scope of using statements

Right now, all of the using statements are at file scope. That's not inherently awful, but it's usually good practice to reduce the scope of such statements where practical. In this case, you could simply move them all within main.

Use const where practical

The assignments variable never changes and could therefore be changed to const as in:

const int assignments = 3;

If you're using a C++11 compiler, you could use the more modern syntax and also declare it constexpr like this:

constexpr int assignments{3};

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the detailed answer - it's really useful. I've implemented a few of the changes you've suggested and I'll work on the others shortly. One quick question for you - what's the difference between using constexpr instead of const? \$\endgroup\$ – Joe Jun 6 '17 at 16:19
  • 1
    \$\begingroup\$ constexpr implies const but has the additional meaning that it can be computed at compile time which often allows the compiler to generate smaller, faster code. See en.cppreference.com/w/cpp/language/constexpr for details. \$\endgroup\$ – Edward Jun 6 '17 at 16:22
  • \$\begingroup\$ Ah, that makes sense. One last question which has just occurred to me. Since I have vector<string> and vector<double> and I iterate over both of them, should I have a separate vec_size variable for the size of each or do they share the same size? \$\endgroup\$ – Joe Jun 6 '17 at 16:37
  • 1
    \$\begingroup\$ They have the same size type (vec_size is a type, not a variable), but generally won't have the same size. (They do in this code, but that's not something one could generally count on.) The cleaner way to deal with this, however, is to use a range-for as @yuri recommended or to use auto. \$\endgroup\$ – Edward Jun 6 '17 at 16:41
  • \$\begingroup\$ Thanks a lot for your help, I really appreciate it. I've given the tick to Yuri's answer because both have been super-helpful and it's a toss-up, but thanks very much for your time and effort :-) \$\endgroup\$ – Joe Jun 7 '17 at 8:50

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.