5
\$\begingroup\$

I have started working on the projects listed here. I want to practice my C++ coding skills. I would love any constructive criticism I can get!

This is for the very first problem, #1 "Grading Program"

#include <iostream>
using std::cout;
using std::cin;

int main()
{
    int UserNumber;
    char Grade;

    cout << "Enter the grade you got on that test: ";
    cin >> UserNumber;
    cout << "\n\n\n";

    if (UserNumber > 89 && UserNumber < 101)
    {
        Grade = 'A';
    }

    else if (UserNumber > 79 && UserNumber < 91)
    {
        Grade = 'B';
    }

    else if (UserNumber > 69 && UserNumber < 81)
    {
        Grade = 'C';
    }

    else if (UserNumber > 59 && UserNumber < 71)
    {
        Grade = 'D';
    }

    else if (UserNumber > -1 && UserNumber < 61)
    {
        Grade = 'F';
    }
    else
    {
        Grade = 'Z';
    }


    switch (Grade)
    {
    case 'A':
        cout << "Perfect! You got a: " << Grade;
        break;
    case 'B':
        cout << "Could've been worse. You got a: " << Grade;
        break;
    case 'C':
        cout << "It's okay I guess, you got a: " << Grade;
        break;
    case 'D':
        cout << "Uggh.. You got a: " << Grade;
        break;
    case 'F':
        cout << "Better luck next time! You got a: " << Grade;
        break;
    case 'Z':
        cout << "Undefined value! Make sure it's a value from 0-100!";
        break;


    }
    cout << "\n\n\n\t\t\t";
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ In CodeReview you are supposed to allow time for answers, not accept one right away. \$\endgroup\$ – CashCow Oct 20 '14 at 10:27
  • \$\begingroup\$ @CashCow No worries, I wouldn't have accepted your answer anyways. \$\endgroup\$ – Chantola Oct 20 '14 at 17:26
  • \$\begingroup\$ Well you would do well to understand my code and learn how to use the standard libraries \$\endgroup\$ – CashCow Oct 20 '14 at 17:31
  • \$\begingroup\$ @CashCow I'm sure its easy to understand code you don't explain, and by the looks of it is much harder than the basic I/O I'm working in right now. I honestly don't even know what your doing, your hypothetically giving a middle schooler calculus. \$\endgroup\$ – Chantola Oct 20 '14 at 17:34
  • 1
    \$\begingroup\$ You realize that some number overlap grades! Example 90 is 'A' then a 'B' \$\endgroup\$ – Martin York Oct 20 '14 at 19:16
5
\$\begingroup\$

There's probably much that can be done, but I'll give it a quick review:

  • Are only integer grades allowed? They can be decimals in real life, and that would make for a more robust grading system.

  • Consider renaming UserNumber to something like numericalGrade. The former sounds like it's for an ID number, not a grade.

  • Do not "list" variables at the top of a function in C++, unless it's absolutely necessary. Here, you can declare UserNumber between the std::cout and std::cin.

  • There's no need for a Z case here. Use a default case instead, which will be reached if no other case is reached.

  • You don't need an explicit return 0 at the end. The compiler will do this return for you at the end of main() as successful termination is always implied here.

\$\endgroup\$
  • \$\begingroup\$ Thank you for all the info, one question. Why should I not "list" variables at the top? My C++ Professor told us that was how we had to do it with all his tests, so I'm wondering why. \$\endgroup\$ – Chantola Oct 20 '14 at 23:01
  • \$\begingroup\$ @Chantola: If he requires it, then that's okay. In general, it's best not to do that because you may lost track of which variables are still used. If you use a variable somewhere, but remove that section, then you can remove that variable easier. \$\endgroup\$ – Jamal Oct 21 '14 at 0:15
  • \$\begingroup\$ that makes sense. For the record, he also makes it mandatory to use using namespace std; and It's already the midterm so I doubt he will change that, but I'll practice it the proper way. Thank you. \$\endgroup\$ – Chantola Oct 21 '14 at 1:59
4
\$\begingroup\$

Ok, this is a range to value issue, and in this case you have very few ranges, but I still would prefer to see the use of a "map". Assuming percentage is an int, you could do something like this:

std::map< int, char > gradeMap;

// initialisation code, but can be read from config etc.
// Stored somewhere.

gradeMap[ 90 ] = 'A';
gradeMap[ 80 ] = 'B';
gradeMap[ 70 ] = 'C';
gradeMap[ 60 ] = 'D';
gradeMap[ 0 ] = 'F';

to look up my grade from a percentage, you can use upper_bound

auto iter = gradeMap.upper_bound( grade );

This will actually get me the iterator one higher than the one I want. Thus if it is "begin" I have a grade 'Z' otherwise I should subtract one from the iterator.

if( iter == gradeMap.begin() )
{
     return 'Z';
}
else
{
     --iter;
     return iter->second;
}

However this is rather annoying. So there are 2 ways we can fix it. Either change the predicate in the map to sort downward. Or simply make our keys negative. We are hiding the implementation so it doesn't matter.

gradeMap[ -90 ] = 'A';
gradeMap[ -80 ] = 'B';
gradeMap[ -70 ] = 'C';
gradeMap[ -60 ] = 'D';
gradeMap[ 0 ] = 'F';

Now we can use lower_bound on -grade. Note it changes from upper_bound before to lower_bound because of the case where our value is equal to the value.

The difference between lower_bound and upper_bound is when the searched value equals the key, and this time we want the cell we are in, not the cell below it, so we want lower_bound.

If our percentage is, say 75 we would now search for -75 which falls between -80 and -70 but the lower bound is the one that has -70 because lower_bound returns the first cell whose value is greater than or equal to the searched value. (upper_bound returns the first cell whose value is strictly greater. It would work for -75 but not for -70 itself).

char get_grade( int percent )
{
     auto iter = gradeMap.lower_bound( -percent );
     if( iter == gradeMap.end() )
         return 'Z';
     else
         return iter->second;
}

It's not really the place here to teach the user the full detail of what std::map is. I will summarise to say firstly that it is one of the main collection classes in the standard library, and is mostly used as an associative array, that is, a unique key on which you look up and find its associated value. Used that way, the key either exists or it doesn't.

However there are occasions where you are not interested in exact key values but ranges, and you can take advantage of the fact that map is a sorted container on its keys and that you can use lower_range and upper_range to find the points where your "key" belongs even if it is not actually in the container. (The only difference between lower_bound and upper_bound is when the key does exist). The fact that it returns an iterator, which can either point to one of the elements or one past the last one.

And that it is logarithmic in its lookup. For this instance where you have just 5 keys the performance doesn't really matter, but it's useful to know in general as part of your general education.

That you can use it to solve this particular kind of problem in C++ is useful. Some languages only use hash-table lookup for associative containers, which can be faster when it is only an exist/not exist situation but not useful for ranges at all.

\$\endgroup\$
1
\$\begingroup\$

In addition to Jamal's answer, I would have refactored getting a literal grade from number and getting a message from literal grade into separate functions. Btw, your boundary conditions overlap, which is probably a bug.

\$\endgroup\$
  • \$\begingroup\$ Not technically a bug because you don't reach the later ifs but semantically incorrect \$\endgroup\$ – CashCow Oct 20 '14 at 10:30
  • \$\begingroup\$ Since you don't know which of boundary conditions is wrong, you cannot claim that this is not a bug. If the first one is wrong, there is a major issue (wrong grade), and if the second one is wrong there is a minor issue (code is superfluous and confusing). A correct program should neither contradict specification (missing/buggy features), nor add unnecessary code (unintended features). A good program also should not be confusing. See Section 2.2.2 Structural Coverage Analysis in this NASA tutorial for some background. \$\endgroup\$ – mkalkov Oct 20 '14 at 11:21
  • \$\begingroup\$ Aside from that the refactor to move the logic out of main is certainly a good idea.. \$\endgroup\$ – CashCow Oct 20 '14 at 11:49
1
\$\begingroup\$

You can try this in the if statement:

if (UserNumber => 90 && UserNumber <= 100)
Grade = 'A';

...and I guess the switch statements are not needed. You can do it like this:

if (UserNumber >= 90 && UserNumber <= 100){

    cout << "Perfect! You got A: " << UserNumber << " Grade";
}
\$\endgroup\$
1
\$\begingroup\$

You don't need the switch statement or the Grade variable. Combine what you put in the switch with what you put in the if block. Like this:

if (UserNumber >= 90) 
    std::cout << "You got an A. Hey, that's pretty good! << '\n';
\$\endgroup\$
-2
\$\begingroup\$

It seems to be poor code, so let's refactor it:

  #include <iostream>
  #include <string>

    char checkGrade(int num){
        char Grade = 'Z';

        if (num > 89 && num < 101) {
            Grade = 'A';
        }
        else if (num > 79 && num < 91) {
            Grade = 'B';
        }
        else if (num > 69 && num < 81) {
            Grade = 'C';
        }
        else if (num > 59 && num < 71) {
            Grade = 'D';
        }
        else if (num > -1 && num < 61) {
            Grade = 'F';
        }

        return Grade;
    };

    string printMessage(char Grade){
        std::string msg;

        switch (Grade)
        {
        case 'A':
            msg =  "Perfect! You got a: "  + Grade;
            break;
        case 'B':
            msg =  "Could've been worse. You got a: " + Grade;
            break;
        case 'C':
            msg =  "It's okay I guess, you got a: " + Grade;
            break;
        case 'D':
            msg =  "Uggh.. You got a: " + Grade;
            break;
        case 'F':
            msg =  "Better luck next time! You got a: " + Grade;
            break;
        case default:
            msg =  "Undefined value! Make sure it's a value from 0-100!";
            break;
        }

        return msg;
    };

    int main()
    {
        int numericalGrade;
        char Grade;

            cout << "Enter the grade you got on that test: ";
            cin >> numericalGrade;
            cout << "\n\n\n";

            // at least one line ...
            cout << printMessage( checkGrade(numericalGrade) );

            cout << "\n\n\n\t\t\t";
            return 0;
        }

You do this with design patterns, offering better reusability.

\$\endgroup\$
  • 3
    \$\begingroup\$ Answers containing no explanation are shunned. See here for tips on improving your answer and writing better ones in the future. \$\endgroup\$ – SirPython Feb 25 '15 at 22:37

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.