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;
}
1 Answer 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.
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?".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"; }
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));
Don't use
std::endl
unless you really need a manual flush, as flushing is expensive, use'\n'
instead. Keep in mind thatstd::cin
andstd::cout
are linked.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.
The second argument to remainder need not be a constant.
You know
num5
could be zero, leading to division by zero, which is undefined behavior?Defer determining the parameters of the challenge until you know what you need. Otherwise, most is useless make-work.
Don't assume input will succeed. Test it.
Just read from
std::cin
if you need to wait for input, that's portable, secure and efficient, avoidstd::system()
.std::cin.get();
return 0;
is implicit formain()
.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.
-
\$\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– Deduplicator2017年07月19日 21:31:28 +00:00Commented Jul 19, 2017 at 21:31
-
\$\begingroup\$ very new appreciated \$\endgroup\$umaxy– umaxy2017年07月20日 23:03:30 +00:00Commented Jul 20, 2017 at 23:03