3
\$\begingroup\$

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.

\$\endgroup\$
3
  • 3
    \$\begingroup\$ You are missing the cases for Lizard and Spock. \$\endgroup\$ Mar 25 at 3:20
  • 2
    \$\begingroup\$ @MathKeepsMeBusy I don't think OP intended to add those. \$\endgroup\$
    – ggorlen
    Mar 25 at 16:30
  • \$\begingroup\$ One very minor point, I recommend spaces around operators. Like user_input == game_options[2]. \$\endgroup\$
    – Almo
    Mar 25 at 18:25

3 Answers 3

5
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ I appreciate the feedback. I thought adding the "using namespace std" would be a more efficient use of code instead of always typing "std::". I might be wrong. Please let me know why you recommend the other way around? \$\endgroup\$ Mar 25 at 9:41
  • \$\begingroup\$ @DalhaDalha See the SO question linked in my article. \$\endgroup\$
    – Harith
    Mar 25 at 13:04
  • \$\begingroup\$ @DalhaDalha stackoverflow.com/questions/1452721/… \$\endgroup\$ Mar 25 at 16:28
  • \$\begingroup\$ @DalhaDalha Best answer: stackoverflow.com/a/1453605/14065 \$\endgroup\$ Mar 25 at 23:09
3
\$\begingroup\$

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().

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback. Some of the stuff u said are a bit hard for me to understand but I'll implement them nevertheless. Appreciate the effort. \$\endgroup\$ Mar 25 at 9:48
3
\$\begingroup\$

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!";
}
\$\endgroup\$
2
  • \$\begingroup\$ I disagree with what you made out of "(user_input + 1) % 3". You replaced 3 by game options. But that is not useful. Consider what happens to your draw, win, lose logic if someone adds a game option. It doesn't work. So why make an abstract logic that works with any number of game options if actually it doesn't? That just suggests a level of abstract logic that doesn't exist. Add lizard and and spock and your code obfuscates the fact that the game isn't working because it outputs stuff that seems correct at first glance. Same problem with using the %3 at all. Imho that's too clever. \$\endgroup\$
    – DonQuiKong
    Mar 24 at 21:25
  • \$\begingroup\$ @DonQuiKong I replaced 3 with std::size(rps::game_options) to make it clear where the otherwise magic number comes from. I'd say that's the recommended approach. If someone adds an option to rock, paper, scissors I would need to know what selecting that option would mean to the game. It's not clear how OPs logic would handle it either so, with the information we have and for a classic RPS, keeping the logic as simple as possible makes it easier to not make mistakes. As most of the time, you code for the problem you actually have. If requirement changes later, one has to deal with that then. \$\endgroup\$
    – Ted Lyngmo
    Mar 24 at 21:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.