7
\$\begingroup\$

I finished the grading exercise from this list of beginner c++ exercises. I have a (probably unfounded) dislike of if elses to dispatch a grade to a note so contrarily to this similar question I implemented a vector of tuples to which I compare the received note. After completion, I realized that it probably amounts to mostly the same thing, if not worse (I am still comparing the value with an if).

I looking for any feedback, but possibly something about:

  • Is the vector of tuple an unnecessary overhead over the simple if/else?
  • Is it actually slower, or does compilation optimizes execution?
  • In this case it probably doesn't matter because the grade set is small, but is this approach considered a good/correct practice if we were to have a bigger set?

#include <iostream>
#include <vector>
#include <tuple>

using namespace std;

char dispatchGrade(int grade) {
    vector<tuple<int, char>> fork {
            tuple<int, char> {90, 'A'},
            tuple<int, char> {80, 'B'},
            tuple<int, char> {70, 'C'},
            tuple<int, char> {60, 'D'},
            tuple<int, char> {0, 'E'},
    };
    for (vector<tuple<int, char>>::size_type i = 0; i != fork.size(); i++) {
        if (grade >= get<0>(fork[i])) {
            return get<1>(fork[i]);
        }
    }
}

int main() {
    int grade;
    char note;
    cout << "Hello, World! enter your grade:" << endl;
    cin >> grade;
    if (!cin.fail() && grade > -1 && grade < 101) {
        cout << "That is a valid grade. ";
        note = dispatchGrade(grade);
        cout << "That grade is good for a " << note << endl;
    } else {
        cout << "That is not a valid grade." << endl;
    }
}

cmake_minimum_required(VERSION 3.8)
project(grades)

set(CMAKE_CXX_STANDARD 17)

set(SOURCE_FILES main.cpp)
add_executable(grades ${SOURCE_FILES})
\$\endgroup\$
  • \$\begingroup\$ How is E a grade? I think you mean F. \$\endgroup\$ – T145 Oct 3 '17 at 18:39
  • 1
    \$\begingroup\$ @T145 I was under the impression that E was the letter that follows D, but maybe schools don't use it, I don't know about that ;) Oh, but I see the exercise really uses F. \$\endgroup\$ – Félix Gagnon-Grenier Oct 3 '17 at 18:41
  • 1
    \$\begingroup\$ When we were at school we had grades A-E. Think that is pretty normal. Anything under a C was a fail and need to be re-taken. Though a C was less than 70% at our school. \$\endgroup\$ – Martin York Oct 3 '17 at 19:32
10
\$\begingroup\$

Code Review:

grade > -1 && grade < 101

That's fine for integers. But you have to consider that somebody can come along behind you and modify your types sometime in the future. If I changed the type of grade to float (not an unreasonable change). Then a grade of -.05 becomes valid. I would change this to be:

grade >= 0 && grade <= 100

!cin.fail()

Checking the state of the file manually is basically never correct (its an anti-pattern). Also this case is also wrong as fail() does not check for end of file, so you could have hit end of file not read anything into grade.

A better check is:

cin.good();

But the more usual check is

cin  // If used in a boolean context (like an if) it will be converted
     // to a boolean based on the call to good.


if (cin && grade >= 0 && grade <= 100)

But even this is not usual. Usually you check the result of the read operation by placing the read into the if statement.

if (cin >> grade && grade >= 0 && grade <= 100)

Note: the operator>> returns a a reference to the stream, so this is functionally the same as the line above. But it imparts more information.

What you are saying here is: if the read works and the grade is in the range [0..100] we succeeded.

Looking at your dispatch. You have a structure that is the same every time you call.

vector<tuple<int, char>> fork {
        tuple<int, char> {90, 'A'},
        tuple<int, char> {80, 'B'},
        tuple<int, char> {70, 'C'},
        tuple<int, char> {60, 'D'},
        tuple<int, char> {0, 'E'},
};

Since this is never modified it should be marked const. Also since you really only want to build the structure once just mark it static that way you only build the structure on the first call. Also there is a convenience function called make_tuple

static const vector<tuple<int, char>> fork
{
        make_tuple(90, 'A'),
        make_tuple(80, 'B'),
        make_tuple(70, 'C'),
        make_tuple(60, 'D'),
        make_tuple(0,  'E')
}; 

This loop is a bit C-Like (though it works).

for (vector<tuple<int, char>>::size_type i = 0; i != fork.size(); i++)

Though I would have used some typedefs to make it more readable:

typedef std::tuple<int, char>  MarkRange;
typedef std::vector<MarkRange> MarkSet;
for (MarkSet::size_type i = 0; i != fork.size(); ++i)  // prefer pre-increment

If you were using C++03 then I would be using iterators:

for (MarkSet::const_iterator i = fork.begin(); i != fork.end(); ++i)

But since C++11 we have had range based for

for (auto const& mark : fork)

Note:

// Since C++11 prefer `using` over typedef
using MarkRange = std::tuple<int, char>;
using MarkSet   = std::vector<MarkRange>;

Using std::endl is not a good idea. Prefer "\n". The difference is that endl flushes the stream. Flushing is usually always wrong. The streams will flush themselves at appropriate times let the code do its work.

Note: The most common complaint about C++ stream is that they are slower than C stream. This is usually down to inappropriate calls to flush that significantly degrades performance when done incorrectly (and manually is usually always incorrect).

Questions:

Is the vector of tuple an unnecessary overhead over the simple if/else?

I like it. Its called Data Driven programing (where the code uses data to define its structure). This is usually considered best practice.

Is it actually slower, or does compilation optimizes execution?

I doubt you could measure the difference.
But if it is important then you should measure it.
People will always have opinions on this; they are not relevant. The only thing that matters is data. BUT it only matters if this is important. Normally I would consider more expressive readable code more important. This code is readable and easy to update (I could even put the data into a config file that is modified seprately).

In this case it probably doesn't matter because the grade set is small, but is this approach considered a good/correct practice if we were to have a bigger set?

Even in bigger list this seems like a better idea.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ A std::array instead of a std::vector can even be a constexpr. \$\endgroup\$ – Zeta Oct 3 '17 at 18:29
  • \$\begingroup\$ A concrete exemple of how to use typedef really connected the dots over doubts I've been having for some time. Thanks for your detailed review! \$\endgroup\$ – Félix Gagnon-Grenier Oct 3 '17 at 18:53
  • \$\begingroup\$ @FélixGagnon-Grenier: Note. Since C++11 it is better to use using rather than typedef. The using as a typedef is much more powerful as you can also template the using. en.cppreference.com/w/cpp/language/type_alias \$\endgroup\$ – Martin York Oct 3 '17 at 18:57
  • 1
    \$\begingroup\$ @Loki: using is not "better" than typedef, it is similar but different. Sometimes those differences are advantageous, sometimes they are crippling. \$\endgroup\$ – Ben Voigt Oct 3 '17 at 20:46
  • 1
    \$\begingroup\$ @BenVoigt: I still use typedef a lot (out of habit). I am trying to change that habit into using using. But I would love to understand when using is not the correct way. Do you have any pointers? \$\endgroup\$ – Martin York Oct 3 '17 at 22:01

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.