4
\$\begingroup\$

This was my assignment.

Write a C++ program that reads in a series of test scores. If the test score is >= 90 && , <= 100; print the letter grade 'A'. If the score is >= 80 but < 90, print the letter grade 'B'. if the test score is >= 70 and < 80, print the letter grade 'C'. If the test score is >= 60 but < 70 print the letter grade 'D' If the test score is < 60 print the letter grade 'F'. Print the score and the corresponding grade. Determine and print the max score, the min score, the average score, and the number of tests. Terminate on a negative test score.

My code works fine but I feel like there's a better way to do it. I talked to a friend and he said since I was reading from a file a array would be better. How would I got about that?

#include <iostream>
#include <fstream>
#include <iomanip>
#include <string>

using namespace std;

int main()
{
    ifstream inFile;

    int Score, MaxScore, MinScore, AvgScore, NumTests, TotalScore, Num;
    string Grade;

    NumTests = 0;
    TotalScore = 0;
    MaxScore = 0;
    MinScore = 100;

    inFile.open("indata6.txt");

    if (!inFile)

    {
        cout << "Failed to find inFile." << endl;
        return 1;
    }

    cout << "Test Scores & Grades" << endl;
    cout << "--------------------" << endl;

    while ( inFile >> Score)

    {
        NumTests = NumTests + 1; // Setting number of tests
        TotalScore = TotalScore + Score; // Adding the Total score for the Avg equation

        if ( Score > MaxScore) // If statements to find Max score
        {
            MaxScore = Score;

            if ( Score >= 90 &&  Score <= 100) // If statements to find letter Grades
            {
                Grade = "A";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 80)
            {
                Grade = "B";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 70)
            {
                Grade = "C";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 60)
            {
                Grade = "D";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 0)
            {
                Grade = "F";
                cout << Score << " " << Grade << endl;
            }

            else
            {
                cout << "Invalid Test Score:" << Score << endl;
                return 2; 
            }

        }

        else if ( Score < MinScore) // If statement to find Min score
        {
            MinScore = Score;

            if ( Score >= 90 &&  Score <= 100) // If statements to find letter grades
            {
                Grade = "A";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 80)
            {
                Grade = "B";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 70)
            {
                Grade = "C";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 60)
            {
                Grade = "D";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 0)
            {
                Grade = "F";
                cout << Score << " " << Grade << endl;
            }

            else 
            {
                cout << "Invalid Test Score:" << Score << endl;
                return 2;
            }
        }

        else // When a score isnt Max or Min
        {   
            if ( Score >= 90 &&  Score <= 100) // More if statements to find letter grade
            {
                Grade = "A";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 80)
            {
                Grade = "B";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 70)
            {
                Grade = "C";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 60)
            {
                Grade = "D";
                cout << Score << " " << Grade << endl;
            }

            else if ( Score >= 0)
            {
                Grade = "F";
                cout << Score << " " << Grade << endl;
            }

            else 
            {
                cout << "Invalid Test Score:" << Score << endl;
                return 2;
            }
        }
    }

    AvgScore = TotalScore / NumTests; // Finding the avg test score

    cout << "Max Score: " << MaxScore << endl; // Printing Max score
    cout << "Min Score: " << MinScore << endl; // Printing Min score
    cout << "Avg Score: " << AvgScore << endl; // Printing Avg score
    cout << "Number of Tests: " << NumTests << endl; // Printing Number of Tests

    return 0;
}
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

There are many places where you can improve on your code.

  • In C++, you try to introduce variables as late as possible. So don't define all those int types in the beginning of your main program. It's only confusing and in general will lead to low maintainability and low performance. In fact, you will end up not needing them at all.

  • You don't need <iomanip> or <string> for anything, so don't include them.

  • Avoid doing using namespace std - this has been raised in many questions on the site. Similarly, you usually want to print '\n' instead of std::endl, which flushes the buffer. For more, see here.

  • Don't force the user of your program to know the internals of your program. If you can't open the file, consider printing something other than "Failed to find inFile". Sure, at least the author of the source code knows what inFile is (that's the name of a variable!), but maybe it would make more sense for the user to say "input file" (but OK, this is nitpicking :-)).

  • If your data set was huge, it would make a lot of sense to count the number of scores and to accumulate the score while reading. In this case, it might be a better idea to just read all the scores into a suitable container (std::vector) and then to process that. In addition, the if-else statements are a mess, as you would probably agree. It's hard to read, difficult to understand, and not easy to modify. So whenever you see a big cluster of if-else statements, ask yourself whether a data driven approach would make sense.

  • A line of code can't be any clearer than cout << "Max Score: " << MaxScore << endl;. Thus, a comment like // Printing Max score is just too verbose. Prefer comments that answer the question "how" and not "what". Good code explains itself via e.g., good naming conventions.

I'd rewrite your program as follows:

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <map>
#include <functional>
#include <algorithm>
#include <numeric>
#include <iterator>

int main()
{
    std::ifstream inFile("indata6.txt");

    if (!inFile)
    {
        std::cout << "Failed to find input file\n";
        return EXIT_FAILURE;
    }

    const std::map<int, char, std::greater<int> > grades = 
    { 
        { 90, 'A' },{ 80, 'B' },{ 70, 'C' },{ 60, 'D' } 
    };

    std::vector<int> scores { 
        std::istream_iterator<int>(inFile), 
        std::istream_iterator<int>{} };

    std::cout << "Test Scores & Grades\n";
    std::cout << "--------------------\n";

    for(auto s : scores)
    {
        auto it = grades.lower_bound(s);
        std::cout << s << " ";

        if (it == grades.cend())
        {
            std::cout << "F\n";
        }
        else
        {
            std::cout << it->second << "\n";
        }
    }

    const auto minmax = std::minmax_element(scores.cbegin(), scores.cend());

    std::cout << "Max Score: " << *(minmax.second) << "\n"; 
    std::cout << "Min Score: " << *(minmax.first) << "\n";
    std::cout << "Avg Score: " << (std::accumulate(scores.cbegin(), scores.cend(), 0.0) / scores.size()) << "\n";
    std::cout << "Number of Tests: " << scores.size() << "\n";
}

A few points from this program:

  • Notice we first try to open the file. If it succeeds, great. If it doesn't, we exit early (with a proper exit code) and avoid possibly allocating memory etc. for variables we never ended up needing.

  • Instead of an if-else mess, we take a data-driven approach powered by std::map. In this way, we just initialize the map to hold the point thresholds for various grades, and then later on do suitable searches into this data structure. It's much easier to maintain!

  • We only read the scores once into a dynamic array (std::vector). This guy holds all the scores and allows us to iterate over it and use standard algorithms on it. Clean and nice.

  • At the end, we do just this: std::minmax_element gives us the smallest and largest element of a range. Similarly, std::accumulate sums over all the elements, which we just divide by the number of elements, conveniently given to us by the size() method of the vector.

  • I would also encourage you to look at functions. You could further separate pieces of the logic into functions. For instance, a natural function would be one that reads an input file into a vector and spits that out.

\$\endgroup\$
0
2
\$\begingroup\$

Points additional to Juho's review:

  • Instead of reading from a compiled-in filename, allow the user to specify the file to read (e.g. as a command argument). Even better, just accept input on std::cin; that gives much greater flexibility (e.g. will allow us to filter or combine sets of scores to pass to our program in a pipeline).

  • Be careful with TotalScore, which is an int, and so could overflow when accumulating relatively few results (e.g. on platforms where int is 16 bits, at just 328 top marks as input). Using unsigned int will double that range; a longer type (such as std::uint32_t) will allow you to accumulate millions of results. For really large input sets, you'll want to keep an incremental mean rather than adding all the inputs - see Incremental calculation of weighted mean and variance by Tony Finch for a good introduction to the method.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Great additional points! I believe that one possible implementation for all kinds of incremental statistics is Boost.Accumulators. \$\endgroup\$
    – Juho
    Commented Mar 4, 2019 at 10:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.