The fact that each case in your switch had a lot of repetition should have raised a red flag. There is a principle called DRY (Don't Repeat Yourself) that allows us to write better code. This is because every repetition is an extra place where you have to change logic if it needs changing. Eventually you'll miss one, and different scenarios will behave differently. It also inflates the size of your code and makes it less readable.
So how do we go about not repeating ourselves? We need to look at the repetitive part, and figure out if we need to put it in a loop, in a function, or just pull it out in your case.
Here is your code, with the repetition pulled out. First, let's discuss the user input. You always need to get it, and it's not dependent on what the computer chose. The user's choice and the computer's choice are mutually exclusive.
Another part that is pulled out is asking to continue. Again, it has nothing to do with what selection the computer makes. You typed the code asking to continue NINE times. That couldn't have felt right.
#include <cstdlib>
#include <ctime>
#include <iostream>
using namespace std;
int main() {
srand((unsigned)time(0)); // CHANGED: Only seed ONCE
char answer;
int rng;
int cont = 0;
do {
// CHANGED: Get user's input up front
cout << "r for rock, p for paper, s for scissors \nplease enter your hand: ";
cin >> answer;
rng = (rand() % 3) + 1;
switch (rng) {
case 1:
switch (answer) {
case 'r':
cout << "The computer used rock! \n";
cout << "It's a draw.\n";
break;
case 'p':
cout << "The computer used paper! \n";
cout << "You lose. \n";
break;
case 's':
cout << "The computer used scissors! \n";
cout << "You win! \n";
break;
}
break;
case 2:
switch (answer) {
case 'r':
cout << "the computer used paper! \n";
cout << "You lose. \n";
break;
case 'p':
cout << "the computer used paper! \n";
cout << "It's a draw. \n";
break;
case 's':
cout << "the computer used paper! \n";
cout << "You win! \n";
break;
}
break;
case 3:
switch (answer) {
case 'r':
cout << "the computer used scissors! \n";
cout << "You win! \n";
break;
case 'p':
cout << "the computer used scissors! \n";
cout << "You lose. \n";
break;
case 's':
cout << "the computer used scissors! \n";
cout << "It's a draw. \n";
break;
}
break;
}
cout << "enter 1 to continue: ";
cin >> cont;
} while (cont == 1);
}
We can see already that the switch cases look a lot cleaner. The logic error you're describing is likely still present. But I'm going to admit laziness for why I'm not checking every individual outcome. We can do better than nested switches.
Let's consider the outcomes: the player wins, loses, or draws. The draw is the easiest to check. Did the player's choice match the computer's? Here's your [gutted] do loop.
do {
// CHANGED: Get user's input up front
// TODO: Naive input validation
cout << "r for rock, p for paper, s for scissors \nyour choice: ";
cin >> answer;
rng = (rand() % 3) + 1;
// 1: rock, 2: paper, 3: scissors
int playerNum;
switch (answer) {
case ('r'):
playerNum = 1;
break;
case ('p'):
playerNum = 2;
break;
case ('s'):
playerNum = 3;
break;
default:
std::cerr << "Shouldn't be here\n";
}
if (playerNum == rng) {
std::cout << "It's a tie!\n";
}
cout << "enter 1 to continue: ";
cin >> cont;
} while (cont == 1);
We can see that it's still a bit clunky because the computer chooses an int, and you take input as a char. This requires some conversion on our part. We can eliminate the need for conversion entirely by just matching the types. I like the idea of the user entering a character (and I think this helps prevent copy/pasta/submit), so I'm going to make the computer choose a character. Since I'm touching how the computer makes its selection, I'm also going to ditch srand()/rand() and use <random>, which is the C++ way of getting random values.
Here's the new code so far:
#include <array>
#include <iostream>
#include <random>
using namespace std;
int main() {
// CPU setup
constexpr std::array<char, 3> choices{'r', 'p', 's'};
std::mt19937 prng(std::random_device{}());
std::uniform_int_distribution<int> rps(0, 2);
char answer;
char cpuChoice; // CHANGED: rng was a bad name
int cont = 0;
do {
// CHANGED: Get user's input up front
// TODO: Naive input validation
cout << "r for rock, p for paper, s for scissors \nyour choice: ";
cin >> answer;
cpuChoice = choices[rps(prng)];
if (answer == cpuChoice) {
std::cout << "It's a tie.\n";
}
cout << "enter 1 to continue: ";
cin >> cont;
} while (cont == 1);
}
Now we can directly compare the user input with the computer's selection, and detect a draw. Let's look at detecting a win.
There are only three scenarios that result in a player win.
player 'r' chooses and cpu chooses 's'
player 'p' chooses and cpu chooses 'r'
player 's' chooses and cpu chooses 'p'
So if we didn't tie, let's see if we won:
if (answer == cpuChoice) {
std::cout << "It's a tie.\n";
} else if ((answer == 'r' && cpuChoice == 's') ||
(answer == 'p' && cpuChoice == 'r') ||
(answer == 's' && cpuChoice == 'p')) {
std::cout << "You win!\n";
}
Let's step back and compare this with your nested switches. To check for a draw required looking at three different pieces of code and making sure all three were correct. I check one piece. Same with a win. When it comes to debugging, this is much simpler to follow and check.
Now, how to handle a loss. Well if you didn't tie, and you didn't win, then you lost. There's no need to check that we did, we know that we did if we made it this far.
if (answer == cpuChoice) {
std::cout << "It's a tie.\n";
} else if ((answer == 'r' && cpuChoice == 's') ||
(answer == 'p' && cpuChoice == 'r') ||
(answer == 's' && cpuChoice == 'p')) {
std::cout << "You win!\n";
} else {
std::cout << "You lose :(\n";
}
And this little block of code determines the outcome of the game. If you want to declare what the computer selected, do it before and outside of this block.
There's lots of other little things that could be changed as well. using namespace std; is bad practice. I used to teach Intro with that line because I thought it was "easing" students in. When it came time to ditch the line in the next semester some students couldn't let it go. So learn to lose it early.
You also don't validate the user input nor do you account for bad input. I'm not talking about anything complicated, just ensuring a good value is entered. It would be best done in its own function.
Here's mine:
template <typename T, typename Container>
void ask_user_with_selection(const std::string& prompt, T& inputVar,
const Container& validOptions) {
bool inputIsInvalid = true;
do {
std::cout << prompt;
std::cin >> inputVar;
if (std::find(validOptions.begin(), validOptions.end(), inputVar) ==
validOptions.end()) {
std::cout << "Please enter a valid choice.\n";
std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
std::cin.clear();
} else {
inputIsInvalid = false;
}
} while (inputIsInvalid);
}
When I get around to learning C++20 concepts, this function will change a bit.
