5
\$\begingroup\$

My code does the job for the most part, but I don't think the two functions that look for the highest and lowest score are the best approach. The instructions tell me to use the int as the return instead of double, but I can't tell how to take this approach.

A particular talent competition has 5 judges, each of whom awards a score between 0 and 10 to each performer. Fractional scores are allowed. The final score of a performer is determined by dropping the highest and lowest score received, then averaging the 3 remaining scores. Should include the following functions:

  • void getJudgeData() - should ask the user for the judge's score, store it in a reference parameter variable, and validate it. Called in main() for each 5 judges.

  • double calcScore() - should calculate and return the average of the 3 scores that remain after dropping the highest and lowest scores the performer received. This function can only be called once by main() and should be passed the 5 scores.

Two additional functions should be called by calcScore(), which uses the returned info to determine which of the scores to drop:

  • int findLowest() - should find and return the lowest of the 5 scores passed to it.

  • int findHighest() - should find and return the highest of the 5 scores passed to it.

#include <iostream>
#include <iomanip>
using namespace std;

// Function prototypes
void getJudgeData(double &someValue);
double calcScore(double a, double b, double c, double d, double e);
double findLowest(double aa, double bb, double cc, double dd, double ee);
double findHighest(double aaa, double bbb, double ccc, double ddd, double eee);

int main ()
{   double scores, avgValue, judge1, judge2, judge3, judge4, judge5;

    // Ask for the score of each judge
    getJudgeData(scores);
    judge1 = scores;
    cout << judge1 << endl;
    getJudgeData(scores);
    judge2 = scores;
    cout << judge2 << endl;
    getJudgeData(scores);
    judge3 = scores;
    cout << judge3 << endl;
    getJudgeData(scores);
    judge4 = scores;
    cout << judge4 << endl;
    getJudgeData(scores);
    judge5 = scores;
    cout << judge5 << endl;

    //Outputs the average value
    avgValue = calcScore(judge1, judge2, judge3, judge4, judge5);
    cout << avgValue;

    return 0;
}


/* getJudgeData asks the user for a score between 1 and 10
   gets store in a reference value and validates the input. */
void getJudgeData(double &someValue)
{   cout << "Enter input of your score from 1-10";
    cin >> someValue;
    if (someValue < 1 || someValue > 10) 
    { cout << "Please enter a valid score from 1-10";
      cin >> someValue;
    }
    else {
      someValue;
    }
}

/* calcScore calculates and return the average of 4 scores that remain after
   dropping the highest and lowest scores of the performer revieced. */
double calcScore(double a, double b, double c, double d, double e)
{   double rtValue;
    double lowScore = findLowest(a, b, c, d, e);
    double highScore = findHighest(a, b, c, d, e);

    rtValue = ((a + b + c + d + e) - (lowScore + highScore))  / 3;

    return rtValue;
}

double findLowest (double aa, double bb, double cc, double dd, double ee)
{   
    double lowestScore;

    if ((aa < bb) && (aa < cc) && (aa < dd) && (aa < ee)) {
    lowestScore = aa;
    }
    else if ((bb < aa) && (bb < cc) && (bb < ee) && (bb < ee)) {
    lowestScore = bb;
    }
    else if ((cc < aa) && (cc < bb) && (cc < dd) && (cc < ee)) {
    lowestScore = cc;
    }
    else if ((dd < aa) && (dd < bb) && (dd < cc) && (dd < ee)) {
    lowestScore = dd;
    }
    else if ((ee < aa) && (ee < bb) && (ee < cc) && (ee < dd)) {
    lowestScore = ee;
    }
    return lowestScore;
}

double findHighest (double aaa, double bbb, double ccc, double ddd, double eee)
{   
    double highestScore;

    if ((aaa > bbb) && (aaa > ccc) && (aaa > ddd) && (aaa > eee)) {
    highestScore = aaa;
    }
    else if ((bbb > aaa) && (bbb > ccc) && (bbb > eee) && (bbb > eee)) {
    highestScore = bbb;
    }
    else if ((ccc > aaa) && (ccc > bbb) && (ccc < ddd) && (ccc > eee)) {
    highestScore = ccc;
    }
    else if ((ddd > aaa) && (ddd > bbb) && (ddd > ccc) && (ddd > eee)) {
    highestScore = ddd;
    }
    else if ((eee > aaa) && (eee > bbb) && (eee > ccc) && (eee > ddd)) {
    highestScore = eee;
    }
    return highestScore;
}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! Part of your question is on-topic, and we can review the code you have provided, but this bit: "The instructions tell me to use the int as the return instead of double, but I can't tell how to take this approach" is asking us for help with coding concepts, and is not a matter for review. \$\endgroup\$
    – Nick Udell
    Nov 3 '14 at 9:39
  • \$\begingroup\$ I will remember that next time. Thank you. \$\endgroup\$
    – RufioLJ
    Nov 3 '14 at 10:12
6
\$\begingroup\$

With C++11, some code may be simplified:

double findLowest (double aaa, double bbb, double ccc, double ddd, double eee)
{
    return std::min({aaa, bbb, ccc, ddd, eee});
}

double findHighest (double aaa, double bbb, double ccc, double ddd, double eee)
{
    return std::max({aaa, bbb, ccc, ddd, eee});
}

But if you use arrays and this algorithm, the code may be simplified even more:

/* calcScore calculates and return the average of 4 scores that remain after
   dropping the highest and lowest scores of the performer revieced. */
double calcScore(const double (&a)[5])
{
    auto it = std::minmax_element(std::begin(a), std::end(a));

    return (std::accumulate(std::begin(a), std::end(a), 0.)) - (*it.first + *it.second))  / 3;
}

int main ()
{
    double judges[5];

    // Ask for the score of each judge
    for (auto& judge : judges) {
        getJudgeData(judge);
        std::cout << judge << std::endl;
    }

    //Outputs the average value
    avgValue = calcScore(judges);
    std::cout << avgValue;

    return 0;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ This looks very good, sadly I still haven't gotten into C++11 and I'm sure I'm required to use the basics. Thank you \$\endgroup\$
    – RufioLJ
    Nov 3 '14 at 9:23
  • 1
    \$\begingroup\$ @RufioLJ: My second sample use mainly mainly algorithm, the only C++11 feature I used is the for-range which can be rewritten for C++03 easily. \$\endgroup\$
    – Jarod42
    Nov 3 '14 at 9:32
4
\$\begingroup\$

You have a little bug in your application. The request had been each of whom awards a score between 0 and 10 but you define a invalid score (someValue < 1 || someValue > 10). So if you take between literaly, invalid should be (someValue < 1 || someValue > 9) or if you take it between with including the invalid should be (someValue < 0 || someValue > 10)

Naming

void getJudgeData(double &someValue);
double calcScore(double a, double b, double c, double d, double e);
double findLowest(double aa, double bb, double cc, double dd, double ee);
double findHighest(double aaa, double bbb, double ccc, double ddd, double eee);  

these parameternames aren't meaningful. You should always use meaningful names for method names, method parameters and also variable names.

\$\endgroup\$
3
  • \$\begingroup\$ You are right! I think there is also another bug that has to do with the findLowest() and findHighest() functions, whenever I input the 5 scores without going from low to high it throws an error. I was hoping to get some info on how to improve those 2 functions that look very forced. \$\endgroup\$
    – RufioLJ
    Nov 3 '14 at 9:26
  • \$\begingroup\$ About the naming, can I use the same parameters in different functions? I think no, right? I also thought this was bad but it was a quick way to prove if my two findLowest() and findHighest() functions worked. \$\endgroup\$
    – RufioLJ
    Nov 3 '14 at 9:27
  • \$\begingroup\$ @RufioLJ, you can use the same parameter names for the different methods. \$\endgroup\$
    – Heslacher
    Nov 3 '14 at 9:28
4
\$\begingroup\$

You can simplify your code greatly, and make it more general-purpose, by using an array for the scores, rather than individual variables. So change e.g.

double scores, avgValue, judge1, judge2, judge3, judge4, judge5;

to

double scores[5], avgValue;

and use a loop to read in the five scores.

You'll also want to change functions such as calcScore so that they take an array rather than separate scores, e.g.

double calcScore(double scores[], int num_scores)
{
    // ...
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ this looks very good. I was forgetting about arrays! I will try using for loops and arrays and report back. Thank you \$\endgroup\$
    – RufioLJ
    Nov 3 '14 at 9:24
2
\$\begingroup\$

Your use of the input routine getJudgeData() is a bit cumbersome. You can read directly into the variable you want.

int main() {
    double judge1, judge2, judge3, judge4, judge5;

    // Ask for the score of each judge
    getJudgeData(judge1);
    std::cout << judge1 << std::endl;
    getJudgeData(judge2);
    std::cout << judge2 << std::endl;
    …
}

But that is repetitive. When you have a collection of variables that are more or less the same as each other, it is likely that an array is appropriate. (Better yet, use a std::vector<double>.)

double calcScore(double scores[], size_t n) {
    …
}

int main() {
    double scores[5];

    // Ask for the score of each judge
    for (int i = 0; i < sizeof(scores) / sizeof(scores[0]); i++) {
        getJudgeData(scores[i]);
        std::cout << scores[i] << std::endl;
    }

    // Output the average after discarding the extremes
    std::cout << calcScore(scores, sizeof(scores) / sizeof(scores[0])) << std::endl;
}

That will easily scale up to any number of judges — just change the 5 to any number.

\$\endgroup\$
1
  • \$\begingroup\$ I haven't gotten into c++11 yet, but I will take it into consideration. I'm in the process of converting some parts with arrays and for loops. I was still hoping to get some feedback on the last 2 functions that look very bad. Thank you \$\endgroup\$
    – RufioLJ
    Nov 3 '14 at 9:28
1
\$\begingroup\$

The function void getJudgeData(double &someValue) has some flaws.

...
cin >> someValue;
if (someValue < 1 || someValue > 10) 
{ cout << "Please enter a valid score from 1-10";
  cin >> someValue;
}
...

One thing that pops out immediately is that this code only allows the user to make one mistake. If the user inputs two incorrect values in a row, it gets accepted regardless. To fix this you can use a while loop instead of an if statement.

The second issue I see is that if the user were to input non numerical values, then your code would crash. To fix this you should add some error checking to see if the input is indeed a number.

void getJudgeData(double &someValue)
{
    std::cout << "Enter input of your score from 0-10" << std::endl;
    while(true) 
    {
        std::string input = "";
        std::cin >> input;
        std::stringstream myStream(input);
        // myStream >> someValue will evaluate false if the str -> double conversion is
        // invalid. If it is valid, the result is stored in someValue.
        // The someValue >= 0 && someValue <= 10 checks will only be evaluated if the
        // first condition is true. Meaning if the input is invalid, you don't have to
        // worry about these checks breaking the program.
        if (myStream >> someValue && someValue >= 0 && someValue <= 10)
        {
            return;
        }
        std::cout << "Invalid number. Enter input of your score from 0-10" << std::endl;
    }
 }
\$\endgroup\$

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.