4
\$\begingroup\$

I was looking for a light weight coding challenge and found this:

In the sport of diving, seven judges award a score between 0 and 10, where each score may be a floating-point value. The highest and lowest scores are thrown out and the remaining scores are added together. The sum is then multiplied by the degree of difficulty for that dive. The degree of difficulty ranges from 1.2 to 3.8 points. The total is then multiplied by 0.6 to determine the diver’s score. Write a computer program that inputs a degree of difficulty and seven judges’ scores, and outputs the overall score for that dive. The program should ensure that all inputs are within the allowable data ranges.

So, I tried to write some modern c++, and I am looking for suggestions to modernize it or is it so basic that this isn't a concept.

#include "pch.h"
#include <iostream>
#include <sstream>
#include <string>

constexpr auto JUDGES = 7;
constexpr auto FUDGE_FACTOR = 0.6f;
constexpr auto MIN_DIFFICULTY = 1.2f;
constexpr auto MAX_DIFFICULTY = 3.8f;
constexpr auto MIN_SCORE = 0.f;
constexpr auto MAX_SCORE = 10.f;

using std::cout;
using std::endl;
using std::cin;
using std::string;
using std::stringstream;

float getInput(float low, float high)
{
    string line;
    float ret;    
    while (std::getline(cin, line))
    {
        stringstream ss(line);
        if (ss >> ret && ss.eof()) {
            if (ret>=low && ret<=high) return ret;
        }
        cout << "invalid input, try again: ";
    }
    return -1.0;
}

int main()
{
    float difficulty;
    float total{ 0. };
    float min{ MAX_SCORE };
    float max{ MIN_SCORE };    

    cout << "please enter degree of difficulty for the dive: ";
    difficulty = getInput(MIN_DIFFICULTY, MAX_DIFFICULTY);
    for (int i = 0; i < JUDGES; ++i) {
        cout << "please enter the judge #"<<i+1<<"'s score: ";
        auto score = getInput(MIN_SCORE, MAX_SCORE);
        if (score < min) min = score;
        if (score > max) max = score;
        total += score;
    }
    total -= (min + max);
    total *= difficulty * FUDGE_FACTOR;
    cout << "The dive scored: " << total << endl;
}

Any thoughts/suggestions will be welcome.

\$\endgroup\$
2
\$\begingroup\$
  • Variables should be initialized with proper values when declared (if not, they should be declared as close to the point of usage as practical). This way there's no need to initialize them to some invalid value, or risk leaving them uninitialized. (difficulty and ret are the main offenders here, but total, min and max should be declared just before the loop using them).

  • I'm not sure there's much point in the using std::foo;. It's probably as much work to keep track of what you're using and update the declarations than just typing std:: where necessary. But it's better than using namespace std;, so that's good. :)

  • The prompt message to the user asking for input could be passed as a parameter to getInput and printed as part of the function.

  • The min and max values could also be shown when requesting an input.


  • Consider splitting the main function into two phases: getting input, calculating the result. This may make the program a little easier to understand.

  • Certain things could be split off into separate functions. e.g. String to float conversion and calculating the overall score.

These last two are really just suggestions though; the original code is very good.

Edited code, with some alternative ways of doing things:

#include <iostream>
#include <sstream>
#include <string>
#include <vector>
#include <algorithm>
#include <cassert>

constexpr auto JUDGES = 7;
constexpr auto FUDGE_FACTOR = 0.6f;
constexpr auto MIN_DIFFICULTY = 1.2f;
constexpr auto MAX_DIFFICULTY = 3.8f;
constexpr auto MIN_SCORE = 0.f;
constexpr auto MAX_SCORE = 10.f;

bool toFloat(std::string const& string, float& value)
{
    std::stringstream ss(string);
    return (ss >> value && ss.eof());
}

bool isInRange(float value, float low, float high)
{
    return (value >= low && value <= high);
}

float getFloatInput(float low, float high, std::string const& prompt)
{
    while (true)
    {
        std::cout << prompt << " [" << low << ", " << high << "]: ";

        std::string line;
        std::getline(std::cin, line);

        auto value = 0.f;
        if (toFloat(line, value) && isInRange(value, low, high))
            return value;

        std::cout << "invalid input!\n";
    }

    assert(false);
    return -1.0f;
}

std::vector<float> getJudgesScores()
{
    auto scores = std::vector<float>();

    for (auto i = 0; i != JUDGES; ++i)
        scores.push_back(getFloatInput(MIN_SCORE, MAX_SCORE, "score #" + std::to_string(i + 1)));

    return scores;
}

float calculateOverallScore(std::vector<float> const& scores, float difficulty)
{
    auto min = MAX_SCORE;
    auto max = MIN_SCORE;
    auto sum = 0.f;

    for (auto score : scores)
    {
        min = std::min(min, score);
        max = std::max(max, score);
        sum += score;
    }

    sum -= (min + max);

    return sum * difficulty * FUDGE_FACTOR;
}

int main()
{
    auto difficulty = getFloatInput(MIN_DIFFICULTY, MAX_DIFFICULTY, "difficulty");
    auto judgesScores = getJudgesScores();
    auto overallScore = calculateOverallScore(judgesScores, difficulty);

    std::cout << "The dive scored: " << overallScore << std::endl;
}
\$\endgroup\$
  • \$\begingroup\$ As an ex-FORTRAN-66 programmer, its difficult for me to not declare variables at the top of a function. I absolutely agree about difficulty. I like the idea of passing the string to the Input function along with displaying min/max values. I initially wrote this using a scores vector, but then realized that this was likely to be generally inefficient. It seems like I should be breaking my code up into smaller chunks. Is returning a vector a common thing? should I be doing that? \$\endgroup\$ – davidbear Oct 29 '18 at 0:04
  • \$\begingroup\$ Using a vector is definitely less efficient. But in this case (storing 7 values) it doesn't matter. Returning a vector like this will result in either a move or NRVO (named return value optimization) which omits the move / copy entirely. It's not so much splitting code into smaller chunks, as giving those chunks names, specifying and naming inputs (function arguments), and limiting scope (local variables). It also benefits maintainability - if you want to change the implementation of toFloat, you can change that function's internals and not worry about the context it's used in). \$\endgroup\$ – user673679 Oct 29 '18 at 8:18
1
\$\begingroup\$
  • Since that MIN_DIFFICULTY and MAX_DIFFICULTY and that MIN_SCORE and MIN_SCORE come in pair, you could use std::pair to store them.

  • You can use std::array instead of std::vector as you know the size of your container at compile time.

  • You can retrieve min and max score in one call to std::minmax or std::minmax_element.
  • Instead of a loop, you can use std::accumulate or std::reduce to adds all scores together.
  • With std::stof you dont have the overhead of std::stringstream (But it's a matter of taste)
  • for inputing float you should use something like this:

(From here)

// #include<limits> //for numeric_limits
//...
float fl;
while(!(std::cin >> fl)) {
  std::cin.clear();
  std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}
\$\endgroup\$
  • \$\begingroup\$ What don't you have with std::stof? I'm curious. ;) \$\endgroup\$ – user673679 Oct 29 '18 at 18:11
  • \$\begingroup\$ ...the overhead of std::stringstream (But it's a matter of taste) -- j'ai été coupé dans mes pensées, sry \$\endgroup\$ – Calak Oct 29 '18 at 18:54

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.