Rock Paper Scissors Game feedback

Question

I made this Rock Paper Scissors game using C++. This is my first time doing it and it took me around 3 hours.

I know that since I am unexperienced, there is probably a much easier and shorter way of making it. Please help me out and give me feedback on things I could improve on.

#include <iostream>
#include <stdlib.h>
#include <time.h>
using namespace std;

int main() {

  char again = 'y';

  while (again == 'y' || again == 'Y') { // Begin of Loop to play again
  
  srand (time(NULL)); 
  char user_input;
  const char game_options[3] = {'R', 'P', 'S'};

  cout<<"WELCOME TO ROCK(R), PAPER(P), SCISSORS(S). PICK YOUR HAND...\n";
  cin >> user_input;

  char rand_letter = rand() % 3;
  char computer_input=game_options[rand_letter];
  cout <<computer_input<<endl;

// All possibilities for user picking Rock(R)
  if (user_input==game_options[0] && computer_input==game_options[0]) {
    cout <<"The Computer Picks Rock(R). It is a Tie!! \n";
  } else if (user_input==game_options[0] && computer_input==game_options[1]) {
    cout <<"The Computer picks Paper(P). You Lose!! \n";
  } else if (user_input==game_options[0] && computer_input==game_options[2]) {
    cout <<"The Computer picks Scissors(S). You Win!! \n";
// All possibilities for user picking Paper(P)
  } else if (user_input==game_options[1] && computer_input==game_options[0]) {
    cout <<"The Computer Picks Rock(R). You Win!! \n";
  } else if (user_input==game_options[1] && computer_input==game_options[1]) {
    cout <<"The Computer picks Paper(P). It is a Tie!! \n";
  } else if (user_input==game_options[1] && computer_input==game_options[2]) {
    cout <<"The Computer picks Scissors(S). You Lose!! \n";
// All possibilities for user picking Scissors(S)
  } else if (user_input==game_options[2] && computer_input==game_options[0]) {
    cout <<"The Computer Picks Rock(R). You Lose!! \n";
  } else if (user_input==game_options[2] && computer_input==game_options[1]) {
    cout <<"The Computer picks Paper(P). You Win!! \n";
  } else if (user_input==game_options[2] && computer_input==game_options[2]) {
    cout <<"The Computer picks Scissors(S). It is a Tie!! \n";
  } else {
    cout << "Wrong Input"; //If any other value is inputed other than the given ones, this will return.
  }
  
  cout << "Do you want to play again? (y/n)" << endl;
  cin >> again;

  } //End the loop

  cout << "Thanks for playing!";
  

  return 0;
}

If you want to checkout the application, the github is here.

As I said, any and all feedback is welcome.

Answer 1

Issues:

My main issue is your large piece of code to decide who wins:

// All possibilities for user picking Rock(R)
  if (user_input==game_options[0] && computer_input==game_options[0]) {
    cout <<"The Computer Picks Rock(R). It is a Tie!! \n";
  } else if (user_input==game_options[0] && computer_input==game_options[1]) {
    cout <<"The Computer picks Paper(P). You Lose!! \n";


    // REMOVED 22 lines for brevity (see original).


    cout <<"The Computer picks Scissors(S). It is a Tie!! \n";
  } else {
    cout << "Wrong Input"; //If any other value is inputed other than the given ones, this will return.
  }

I would use a more data driven approach. You can encode who won as a table accessed by the player and computers move.

enum Thorable { Rock, Paper, Scissors};
enum State.   { Draw, Win,   Loose};

State playerState[][] = {
       // Computer has:          Rock.  Paper. Scissors
       /* Player has Rock*/     {Draw,  Loose, Win      },
       /* Player has Paper*/    {Win,   Draw,  Loose    },
       /* Player has Scissors*/ {Loose, Win,   Draw     }};        


State playerHas = playerState[playersMove][ComputerMove];

---

For the love of Kernighan and Ritchie. Please use good formatting in your code. Reading code can be non-trivial, don't make it harder with non-standard indenting.

General Coding advice:

Don't do this:

using namespace std;

Good articles on SO tell you all about the issues. But it is not that hard to simply place std:: in front of all standard library objects/functions.

---

You can simplify this:

  while (again == 'y' || again == 'Y') { // Begin of Loop to play again

Why not:

  while (std::tolower(static_cast<unsigned char>(again)) == 'y') {

---

Yes the C random number library is easy to use. And for trivial games like this even I just use this (while I am developing the code). But you should probably use the C++ random number libraries. They are much better defined in terms of what they produce (the C ones have a lot of known issues). So once you have the logic all worked out please replace with the C++ versions.

  srand (time(NULL)); 
  ...
  char rand_letter = rand() % 3;

---

We have enum for this.

  const char game_options[3] = {'R', 'P', 'S'};

---

No need to flush manually.

  cout << "Do you want to play again? (y/n)" << endl;
  cin >> again;

The standard out std::cout and std::cin are linked. If you read from the standard input the standard output is automatically flushed first. In nearly all cases that you need to flush, the standard libraries will do it for you. So you manually flushing is not needed.

I would note that manually flushing the stream is the cause of most peoples complaints that the C++ output is much slower than the C output. By simply not flushing you remove 90% of these issues.

Answer 2

Your indentation is all over the place. Consider using GNU Indent or clang-format to automatically format it.

What's the problem with "using namespace std;"?.

When using C headers in C++, should we use functions from std:: or the global namespace?.

endl is almost never what you require. It flushes the underlying buffer along with inserting a newline. Simply use a '\n'.

rand() returns an int, you're using a char to store its value.

You're not validating if the input was in the correct range or even numerical.

if (!std::cin) {
    std::cerr << /* Error message here. */;
    return EXIT_FAILURE; 
}

Modern day C++11 example replacement for the rand().

The cascade of if/else can be rewritten better:

#if 0
 if (user_input==game_options[0] && computer_input==game_options[0]) {
    cout <<"The Computer Picks Rock(R). It is a Tie!! \n";
  } else if (user_input==game_options[0] && computer_input==game_options[1]) {
    cout <<"The Computer picks Paper(P). You Lose!! \n";
  } else if (user_input==game_options[0] && computer_input==game_options[2]) {
    cout <<"The Computer picks Scissors(S). You Win!! \n";
#else
  if (user_input == game_options[0]) {
     if (computer_input == game_options[1]) {
         ....
     } 
  } else if (user_input == game_options[1]) {
     if ...
#endif

Consider moving this code to another function. Perhaps run_game() or play_game().

Answer 3

Your program is fine as I couldn't spot any logic flaws in it. However, making sure that there are no logic flaws took me a little more time than it would have done had you used the fact that:

• If (user_input + 1) % 3 is equal to computer_input then it's a loss.

To see how that works you could make a mental model like this:

• R + 1 == P
• P + 1 == S
• S + 1 == R

Other improvements could be to use a pseudo random number generator from <random> and to make sure that user input is case insensitive and that it aborts the game in case the input stream is closed.

You could also replace std::endl with '\n' since you rarely need to flush the output, which is what std::endl adds to a plain \n.

Here's putting those ideas into an example of what it could look like:

#include <array>
#include <cstring>
#include <iostream>
#include <random>

namespace rps {
inline constexpr std::size_t invalid_hand = static_cast<std::size_t>(-1);
static constexpr char game_options[] = {'R', 'P', 'S'};

inline std::size_t option_index(const char hand) {
    const char* ptr = static_cast<const char*>(std::memchr(
        game_options, std::toupper(static_cast<unsigned char>(hand)),
        std::size(game_options)));
    if (ptr == nullptr) return invalid_hand;
    return static_cast<std::size_t>(ptr - game_options);
}
}  // namespace rps

int main() {
    std::mt19937 rnd(std::random_device{}());
    std::uniform_int_distribution<unsigned> dist(
        0, std::size(rps::game_options) - 1);

    std::cout << "WELCOME TO ROCK(R), PAPER(P), SCISSORS(S). ";
    char again;
    do {
        std::cout << "PICK YOUR HAND...\n";
        std::size_t user_input;
        if (char user; std::cin >> user && (user_input = rps::option_index(
                                                user)) != rps::invalid_hand) {
            auto computer_input = dist(rnd);
            std::cout << "Computer chose " << rps::game_options[computer_input] << '\n';

            // simplified game logic:
            if (computer_input == user_input) {
                std::cout << "It is a Tie";
            } else if ((user_input + 1) % std::size(rps::game_options) ==
                       computer_input) {
                std::cout << "You loose!";
            } else {
                std::cout << "You win!";
            }
        } else {
            if (std::cin.eof()) break; // the input stream was closed
            std::cout << "Erroneous input!";
        }

        std::cout << "\nDo you want to play again? (y/n)\n";
    } while ((std::cin >> again) && // check that the user actually inputs something
             std::toupper(static_cast<unsigned char>(again)) == 'Y');

    std::cout << "Thanks for playing!";
}