First C++ Program: Race Car Driver Points Calculator

Question

Task:

Write a program that is given three numbers corresponding to the number of times a race car driver has finished first, second, and third. The program computes and displays how many points that driver has earned given 5 points for a first, 3 points for a second, and 1 point for a third place finish.

Code:

#include <iostream>
#include <array>
#include <cstdlib>
namespace /* unnamed */ {
 int compute_points(const std::array<int, 3> &counts)
 {
 /* [0] --> 5 points each.
 * [1] --> 3 points each.
 * [2] --> 1 point each.
 */
 return (counts[0] * 5) + (counts[1] * 3) + counts[2]; // Overflow?
 }
}
int main()
{
 std::array<int, 3> counts{};
 std::cout << "Enter the number of first, second and third place finishes: ";
 std::cin >> counts[0] >> counts[1] >> counts[2]; // Error-checking? 
 
 std::cout << "Total points: " << compute_points(counts) << " \n";
 return EXIT_SUCCESS;
}

Review Requests:

General coding comments, bad practices, style et cetera.

Answer 1

Firstly, let's answer the questions:

 return (counts[0] * 5) + (counts[1] * 3) + counts[2]; // Overflow?

Given that int must be able to represent at least -32767 to +32767, a driver would have to participate in over 6,000 races to overflow int, so this is probably sufficient for our purposes. But we can easily double that by using unsigned int, since we don't expect any negative counts.

 std::cin >> counts[0] >> counts[1] >> counts[2]; // Error-checking? 

Yes, we require some error checking here. It can be as simple as

if (!std::cin) {
 std::cerr << "Input failed\n";
 return EXIT_FAILURE;
}

---

When designing a program like this, it's a good idea to consider which requirements are rigid and which are arbitrary values which could change if we apply this to a different competition (perhaps next year's contest?).

In our case, the things that could be changed in a future version include:

• how many scoring positions are counted, and
• the specific scores for each position.

We should endeavour to make it easy for a future maintainer to be able to change either of these properties. At present, changing the number of scoring positions is particularly onerous, as it requires changing how we read inputs and perform the calculation.

If we make an array of the scores for each position, that can help us:

 static constexpr auto scores =
 std::to_array({5u, 3u, 1u}); // first, second, ...

Then we can derive the number of scoring places:

 static constexpr auto places = scores.size();

And we can define a type to represent a driver's results summary, and use that in our function signature:

 using results = std::array<unsigned, places>;
 unsigned compute_points(const results &counts)

---

When we implement the function, it's good to have a knowledge of the standard algorithms library. In our case, we're performing a dot-product of two vectors (in mathematical terms). That corresponds to std::inner_product(), declared in <numeric>:

 unsigned compute_points(const results &counts)
 {
 return std::inner_product(scores.begin(), scores.end(),
 counts.begin(), 0u);
 }

---

We'll want the main() function to read the correct number of results values. We can use a loop for (auto i = 0uz; i < places; ++i). But it's instructive to see how we can use the standard library to avoid hand-writing the loop.

We'll use the std::copy() algorithm with a std::istream_iterator to read the values:

#include <algorithm>
#include <iterator>

 std::copy_n(std::istream_iterator<unsigned>(std::cin),
 places, counts.begin());

---

Modified program

Here's the version incorporating these improvements that make future adjustments a simple matter of changing the scores list:

#include <algorithm>
#include <array>
#include <cstdlib>
#include <iterator>
#include <numeric>
#include <iostream>
namespace /* unnamed */ {
 static constexpr auto scores =
 std::to_array({5u, 3u, 1u}); // first, second, ...
 static constexpr auto places = scores.size();
 using results = std::array<unsigned, places>;
 unsigned compute_points(const results &counts)
 {
 return std::inner_product(scores.begin(), scores.end(),
 counts.begin(), 0u);
 }
}
int main()
{
 std::cout << "Enter the number of first, second and third place finishes: ";
 results counts{};
 std::copy_n(std::istream_iterator<unsigned>(std::cin),
 places, counts.begin());
 if (!std::cin) {
 std::cerr << "Input failed\n";
 return EXIT_FAILURE;
 }
 std::cout << "Total points: " << compute_points(counts) << " \n";
}

• anon: Good answer, but I disagree with using unsigned numbers anywhere they're not required for compatibility with someone else's bad decision to use them. In this case, they're a particularly bad choice because whilst you may not have negatives in the calculation, one of the most common operations to do with the points of drivers is to look at the difference in points between drivers and that can be negative.
• Toby Speight: The decision whether to use signed or unsigned can be more nuanced than that, as I'm sure you know (e.g. whether overflow is UB can be important). But good point about the difference being a possible use-case.
• Madagascar: @TobySpeight That's a lot to digest at once, but thank you for introducing me to the standard library. Is there a reason you didn't use noexcept and constexpr for compute_points()?
• Toby Speight: No reason - you should be able to add those attributes.
• anon: @Harith: constexpr will only make any difference to performance if the values passed into compute_points() are constant at compile time. Is that the expected usage of compute_points()? It doesn't seem likely.

Answer 2

I agree with Spectral Dev’s suggestion to use named constexpr constants for magic numbers.

The Type of a Score

However, I suggest a different solution for types. It makes the code more readable and maintainable to call the type that holds a score something like score. You also sometimes will want to change every score in the program to something like uint16_t or uint32_t, conveniently, by editing one very simple line of code.

The solution that gives you all this is:

using score = int;

Or, using the older C syntax:

typedef int score;

You then have

score compute_points(const std::array<score, 3> &counts)

And replace all other int that represent scores to score. You will thank yourself later for not having to go through a bigger program than this and reverse-engineer which int variables are scores, and which are something else and must not change type.

The Helper Function

Your function could be constexpr, which allows it to be used inside other constexpr expressions, for metaprogramming, and generally helps the optimizer out.

It can’t ever throw an exception, so you should also declare it noexcept.

You declare it in an anonymous namespace, which some style guides recommend. That’s unnecessary in this example, but perfectly fine. I personally never stopped using static, myself. The keyword was un-deprecated back in 2009, so there’s officially no reason not to use it.

• Madagascar: I was going by this document david.tribble.com/text/cdiffs.htm#C++-vs-C whilst writing this program, where it mentioned that static is deprecated. It is good to see that isn't, thank you.

Answer 3

Magic number

You should not use magic numbers. You added comments because you knew you had magic number. You can use constexpr to not have magic number and not lose performance.

namespace /* unnamed */ {
 int compute_points(const std::array<int, 3> &counts)
 {
 constexpr int SCORE_A = 5;
 constexpr int SCORE_B = 3;
 constexpr int SCORE_C = 1;
 return counts[0] * SCORE_A + counts[1] * SCORE_B + counts[2] * SCORE_C;
 }
}

Good practice -- constexpr

Your "compute_points(...)" function uses std::array and does not perform memory allocation. So you need to define your function with constexpr keyword. This will allow your compiler to make optimizations and allow your function to be resolved at compile time. There is no downside to using constexpr. Because you use std::cin your function won't be resolved at compile time. However it's always a good practice to use constexpr on this function. If you want to know more about it --> https://en.cppreference.com/w/cpp/language/constexpr

Good practice -- template

There is no good reason why your function uses int instead of int8_t, int16_t, ... You should make template function. Then you can call your function with any integer type, even float, double, ...

Code

In the end your function should be

namespace /* unnamed */ {
 template<class T>
 constexpr int compute_points(const std::array<T, 3> &counts) {
 constexpr T SCORE_A = 5;
 constexpr T SCORE_B = 3;
 constexpr T SCORE_C = 1;
 return counts[0] * SCORE_A + counts[1] * SCORE_B + counts[2] * SCORE_C;
 }
}
int main()
{
 std::array<int, 3> counts{};
 std::cout << "Enter the number of first, second and third place finishes: ";
 std::cin >> counts[0] >> counts[1] >> counts[2]; // Error-checking? 
 
 std::cout << "Total points: " << compute_points<int>(counts) << " \n";
 return EXIT_SUCCESS;
}

• Toby Speight: SCORE_A, SCORE_B, SCORE_C is barely an improvement on the raw numbers. And is there really value in the template that's instantiated for only one type (as opposed to, say, using Count = int;?
• anon: I'd argue SCORE_A, B, C is worse than magic numbers. score_1st_place, etc. would be okay, but honestly this should be an array giving scores for places, probably configurable.