Rock Paper Scissors sometimes give the wrong result-CodePudding

Newbie to programming here. Today I tried to make a rock, paper, scissors game to practice rng in c . I've already checked the code multiple times and made sure there is no error or nothing overlaps but sometimes it still displays the wrong outcome. Is it due to my use of switches? Here is the code:

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

using namespace std;

int main() {

    char answer;
    int rng;
    int cont = 0;

    do {
        srand((unsigned)time(0));
        rng = (rand() % 3)   1;

        switch (rng) {
        case 1:
            cout << "r for rock, p for paper, s for scissors \n";
            cout << "please enter your hand: ";
            cin >> answer;
            switch (answer) {
            case 'r':
                cout << "The computer used rock! \n";
                cout << "It's a draw.\n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            case 'p':
                cout << "The computer used paper! \n";
                cout << "You lose. \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            case 's':
                cout << "The computer used scissors! \n";
                cout << "You win! \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            }break;
        case 2:
            cout << "r for rock, p for paper, s for scissors \n";
            cout << "please enter your hand: ";
            cin >> answer;
            switch (answer) {
            case 'r':
                cout << "the computer used paper! \n";
                cout << "You lose. \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            case 'p':
                cout << "the computer used paper! \n";
                cout << "It's a draw. \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            case 's':
                cout << "the computer used paper! \n";
                cout << "You win! \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            }break;
        case 3:
            cout << "r for rock, p for paper, s for scissors \n";
            cout << "please enter your hand: ";
            cin >> answer;
            switch (answer) {
            case 'r':
                cout << "the computer used scissors! \n";
                cout << "You win! \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            case 'p':
                cout << "the computer used scissors! \n";
                cout << "You lose. \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            case 's':
                cout << "the computer used scissors! \n";
                cout << "It's a draw. \n";
                cout << "enter 1 to continue: ";
                cin >> cont;
                break;
            }break;
        }
    } while (cont == 1);
}

CodePudding user response：

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.

CodePudding user response：

You have a few things that are awkward.

First, the reason you don't care for your results -- Compare the code you did for case 1 vs. cases 2 and 3. You'll notice they're different. I believe case 1 is the one that's broken.

Next, you can make your code simpler. I'm going to show you an intermediate step. This isn't how I would do my final version, but it's an evolution towards it. My important changes are at the front and the back. I also simplified the inner switch statements.

    // This code is the same for all 3 cases, so pull it out
    // and only do it once.
    cout << "r for rock, p for paper, s for scissors \n";
    cout << "please enter your hand: ";
    cin >> answer;

    switch (rng) {
    case 1:
        cout << "The computer used rock! \n";
        switch (answer) {
        case 'r':
            cout << "It's a draw.\n";
            break;
        case 'p':
            cout << "You lose. \n";
            break;
        case 's':
            cout << "You win! \n";
            break;
        }
        break;
    case 2:
        cout << "the computer used paper! \n";
        switch (answer) {
        case 'r':
            cout << "You lose. \n";
            break;
        case 'p':
            cout << "It's a draw. \n";
            break;
        case 's':
            cout << "You win! \n";
            break;
        }break;
    case 3:
        cout << "the computer used scissors! \n";
        switch (answer) {
        case 'r':
            cout << "You win! \n";
            break;
        case 'p':
            cout << "You lose. \n";
            break;
        case 's':
            cout << "It's a draw. \n";
            break;
        }break;
    }

    // This code is also the same. Pull it out.
    cout << "enter 1 to continue: ";
    cin >> cont;

Now, this isn't quite what I would do and still stick to your basic structure. I would write a little code with the results and then get rid of a ton of your nested switch statements, but that's more than I want to rewrite.