2
\$\begingroup\$

This is code for an assignment.

Is there extra code I could have avoided using? I'm currently learning C++, so I am limited to my resources, but I do feel like I gave myself extra work. Are there any tips so I can be more efficient at this?

choice is menu
answer in input
quit is outside the menu option. it is part of the do-while loop
dont completely understand the time commands

random number generators
while is used to keep num1 divisable by num6
assigned variables to each operation

sorry for no comments on code

#include<iostream>
#include <cstdlib> 
#include <ctime>

using namespace std;


int main() {

int choice,
answer;

const int quit = 5;


unsigned seed = time(0);
srand(seed);



do
{

int num1 = rand() % 10,
    num2 = rand() % 10,
    num3 = rand() % 11,
    num5 = rand() % 9,
    num4 = num5 * (rand() % 10 + 1),
    num6 = rand() % 10;

while (num6 > num1)
{
    num6 = rand() % 10;
}



int answer1 = num1 + num2,
    answer2 = num1 - num6,
    answer3 = num3 * num1,
    answer4 = num4 / num5;


cout << "Menu\n"
    << "1. Addition problem\n"
    << "2. Subtraction problem\n"
    << "3. Multiplication problem\n"
    << "4. Division problem\n"
    << "5. Quit this program\n\n"
    << "Enter your choice (1-5): ";
cin >> choice;

while (choice > 5 || choice < 1)
{
    cout << "The valid choices are 1, 2, 3, 4, and 5. Please choose: ";
    cin >> choice;
    cout << endl;
}
    switch (choice)
    {
    case  1: cout << endl;
        cout << "  " << num1 << endl;
        cout << "+ " << num2 << endl;
        cout << " ---" << endl;
        cout << "  ";
        cin >> answer;

        if (answer == answer1)
        {
            cout << endl;
            cout << "Congratulations! That's right." << endl;
            cout << endl;
        }
        else
        {
            cout << endl;
            cout << "Sorry, the correct answer is " << answer1 << "." << endl;
            cout << endl;
        }
        break;
    case  2: cout << endl;
        cout << "  " << num1 << endl;
        cout << "- " << num6 << endl;
        cout << " ---" << endl;
        cout << "  ";
        cin >> answer;

        if (answer == answer2)
        {
            cout << endl;
            cout << "Congratulations! That's right." << endl;
            cout << endl;
        }
        else
        {
            cout << endl;
            cout << "Sorry, the correct answer is " << answer2 << "." << endl;
            cout << endl;
        }
        break;
    case  3: cout << endl;
        cout << "  " << num1 << endl;
        cout << "* " << num3 << endl;
        cout << " ---" << endl;
        cout << "  ";
        cin >> answer;

        if (answer == answer3)
        {
            cout << endl;
            cout << "Congratulations! That's right." << endl;
            cout << endl;
        }
        else
        {
            cout << endl;
            cout << "Sorry, the correct answer is " << answer3 << "." << endl;
            cout << endl;
        }
        break;
    case  4: cout << endl;
        cout << "  " << num4 << endl;
        cout << "/ " << num5 << endl;
        cout << " ---" << endl;
        cout << "  ";
        cin >> answer;

        if (answer == answer4)
        {
            cout << endl;
            cout << "Congratulations! That's right." << endl;
            cout << endl;
        }
        else
        {
            cout << endl;
            cout << "Sorry, the correct answer is " << answer4 << "." << endl;
            cout << endl;
        }
        break;
    }

} while (choice != quit);
{
cout << "Thank you for using math tutor!" << endl;
cout << endl;
}

system("PAUSE");
return 0;

}
\$\endgroup\$
1
\$\begingroup\$
  1. Fix your indentation. Most of the time when you open a block, you add a level of indentation, but unfortunately not always. Do it consistently and your code will be much more readable.

  2. Don't ever use using namespace std;. Yes, book-authors are often forced by publishers to sacrifice on the altar of brevity whatever the consequences, but you aren't.
    Read "Why is “using namespace std;” considered bad practice?".

  3. Yes, you can use a single empty line in a function to graphically segregate blocks doing different things. Don't use multiple lines though, and consider whether extracting it into a well-named function might make things more readable, or at least allows you to reuse code.
    One function you really should extract is this, which will be called by all challenges:

    static void do_challenge(char op, int n1, int n2, int result) {
        std::cout << "\n  " << n1 << '\n' << op << ' ' << n2 << "\n ---\n  ";
        int answer;
        if ((std::cin >> answer))
            if (answer == result)
                std::cout << "\nCongratulations! That's right.\n\n";
            else
                std::cout << "\nSorry, the correct answer is " << result << ".\n\n";
    }
    
  4. Don't add a new variable if you can simply inline its initialization, unless it makes things more readable. That should be:

    srand(time(0));
    
  5. Don't use std::endl unless you really need a manual flush, as flushing is expensive, use '\n' instead. Keep in mind that std::cin and std::cout are linked.

  6. Don't stream multiple string- and character-literals in a row, concatenate them and stream a single string-literal. Two string-literals only separated by whitespace (including newlines) are concatenated by the compiler.

  7. The second argument to remainder need not be a constant.

  8. You know num5 could be zero, leading to division by zero, which is undefined behavior?

  9. Defer determining the parameters of the challenge until you know what you need. Otherwise, most is useless make-work.

  10. Don't assume input will succeed. Test it.

  11. Just read from std::cin if you need to wait for input, that's portable, secure and efficient, avoid std::system().

    std::cin.get();
    
  12. return 0; is implicit for main().

  13. Regarding "sorry for no comments on code", I didn't actually miss them. There wasn't a lot of reasoning to explain, and at least you didn't restate your code.

\$\endgroup\$
  • \$\begingroup\$ If my answer helped you, consider accepting it. You don't have to, and you shouldn't do so unless you think your code got well-reviewed. (Only posting this because you seem new to the site.) \$\endgroup\$ – Deduplicator Jul 19 '17 at 21:31
  • \$\begingroup\$ very new appreciated \$\endgroup\$ – umaxy Jul 20 '17 at 23:03

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.