Writing a clean implementation of rock–paper–scissors game in C++

Question

I have coded a rock–paper–scissors game in C++, and it works exactly how I want it to. However, I was wondering if there is a way to simplify the code and clean it up. It is for a school project, so the void function has to be included in the code, but I feel like the while & if functions could somehow be simplified.

#include <iostream>
#include <cstdlib>
#include <string>

using namespace std;

    //defining my variables
    int getUserChoice, getComputerChoice;
    const int Rock = 1, Paper = 2, Scissors = 3, Exit = 4;
    char reply;

void determineWinner()
{
        //random computer choice
        getComputerChoice = rand() % 3;

        //displays the users input choice
        if (getUserChoice == 1)
         {
            cout << "You chose Rock." << endl;
         }

         else if (getUserChoice == 2)
        {
            cout << "You chose Paper." << endl;
        }

        else if (getUserChoice == 3)
        {
            cout << "You chose Scissors." << endl;
        }

        if (getUserChoice == 4)
        {
            cout << "You have chosen to exit the game. The game will now end.";
        }

        //display the computers input choice
        if (getComputerChoice == 1)
         {
            cout << "The computer chose Rock." << endl;
         }

         else if (getComputerChoice == 2)
        {
            cout << "The computer chose Paper." << endl;
        }

        else if (getComputerChoice == 3)
        {
            cout << "The computer chose Scissors." << endl;
        }

       //determines winner
        while (
                (getUserChoice == 2 && getComputerChoice == 3)||
                (getUserChoice == 3 && getComputerChoice == 1)||
                (getUserChoice == 1 && getComputerChoice == 2)
                )

            {
                cout << "I'm sorry, the computer has beat you." << endl << endl;
                break;
            }

        while (
                (getUserChoice == 1 && getComputerChoice == 3)||
                (getUserChoice == 2 && getComputerChoice == 1)||
                (getUserChoice == 3 && getComputerChoice == 2)
                )

            {
                cout << "Congratulations, you win!" << endl << endl;
                break;
            }

        while (
                (getUserChoice == getComputerChoice)
                )
            {
                cout << "This game is tied. " << endl << endl;
                break;
            }
}

int main()
{
    //displays a description of the game
    cout << "Rock, Paper, Scisscors Game!"  << endl << endl;
    cout << "You will be playing against the computer." << endl;
    cout << "Rock beats Scissors; Paper beats Rock; Scissors beats Paper." << endl << endl;
    cout << "------------------------------------------------------------------" << endl << endl;
    cout << "Please choose from the following" << endl;
    cout << "1. Rock" << endl;
    cout << "2. Paper" << endl;
    cout << "3. Scissors" << endl;
    cout << "4. Quit" << endl << endl;

    //loop sequence that allows player to play again if their input is not 4
    while (getUserChoice != 4)
    {
        // play begins
        cin >> reply;
        cout << endl;

        //assinging the numbers to getUserChoice
        switch (toupper(reply))
            {
                case '1':
                getUserChoice = 1;
                break;

                case '2':
                getUserChoice = 2;
                break;

                case '3':
                getUserChoice = 3;
                break;

                case '4':
                getUserChoice =4;
                break;
            }

    //calls the void function to display the winner
    determineWinner();

    }

    return 0;
}

Answer 1

Programming is the art of detecting and removing duplication.

    //display the computers input choice
    if (getComputerChoice == 1)
     {
        cout << "The computer chose Rock." << endl;
     }

     else if (getComputerChoice == 2)
    {
        cout << "The computer chose Paper." << endl;
    }

    else if (getComputerChoice == 3)
    {
        cout << "The computer chose Scissors." << endl;
    }

We could write this more simply with a helper function:

std::string gesture(int x) {
    if (gesture == 1) return "Rock";
    if (gesture == 2) return "Paper";
    if (gesture == 3) return "Scissors";
    assert(false);  // this should never happen
}

and then back in main:

std::cout << "The computer chose " << gesture(getComputerChoice) << ".\n";

Notice also that I'm fully qualifying my identifiers out of namespace std — this is a good practice. You should do it. And I'm not wasting time typing out std::endl when \n does the exact same thing but shorter and faster.

---

        while (
            (getUserChoice == 2 && getComputerChoice == 3)||
            (getUserChoice == 3 && getComputerChoice == 1)||
            (getUserChoice == 1 && getComputerChoice == 2)
            )

        {
            cout << "I'm sorry, the computer has beat you." << endl << endl;
            break;
        }

Two things: First, the use of while (cond) { thing; break; } to mean if (cond) { thing; } is so ridiculous that I wonder if you're trolling. (Odds are against it; but still, why would you write an if as a while and then ask real people to review it for style?)

Second, this would be a great place to show your understanding of the structure of the game. 3 beats 2, 2 beats 1, 1 beats 3. What arithmetic operation could we use here to encode all of those results in a single expression?

...Well, first, let's detect and remove repetition.

if (beats(getComputerChoice, getUserChoice)) {
    std::cout << "I'm sorry, the computer has beat you.\n\n";
} else if (beats(getUserChoice, getComputerChoice)) {
    std::cout << "Congratulations, you win!\n\n";
} else {
    std::cout << "This game is tied.\n\n";
}

Okay, now how do we write the beats function?

bool beats(int a, int b) {
    return ((a == 3 && b == 2) || (a == 2 && b == 1) || (a == 1 && b == 3));
}

But we can use arithmetic instead. a beats b exactly when a is one greater than b, modulo 3.

bool beats(int a, int b) {
    return (a - 1) == b % 3;
}

No comment on the input/output part.

Answer 2

You can get rid of the while loops that break after the first iteration, and replace them with if.

You can also get rid of some if statements, like you say. Consider

    //display the computers input choice
    if (getComputerChoice == 1)
     {
        cout << "The computer chose Rock." << endl;
     }

     else if (getComputerChoice == 2)
    {
        cout << "The computer chose Paper." << endl;
    }

    else if (getComputerChoice == 3)
    {
        cout << "The computer chose Scissors." << endl;
    }

It would be cleaner to use a switch statement:

switch (getComputerChoice) {
    case 1:
         cout << "The computer chose Rock." << endl;
         break; // breaking is important
    case 2: 
         // Case code here
    case 3: 
         // Case code here
    default:
         // the switch statement reminds you that you should consider what happens if the input is something else
}

The switch statement is less code for the same thing, and hence easier readable. Alternatively, you could also use a list of the possible strings, and only write the code for printing once. That feels more elegant to me, but makes the code slightly harder to understand if read by novice co-programmers.

string choices[] = {"Rock", "Paper", "Scissors"};
//display the computers input choice
cout << "The computer chose " << choices[getComputerChoice-1] << "." << endl;

Note the minus one because you start counting from 1 instead of 0.

For determining whether the user won, you can use modulo computation instead of typing out all the relations. It is up to you whether you prefer more readable code (like you wrote) or code that is more easily modified for more numbers. It is not neccessarily better, but I thought I'd point out the possibility.

The switch case in main() for getting the user input as integer can be replaced with

//assinging the numbers to getUserChoice
getUserChoice = reply - '0';

I prefer this because of its simplicity and less code means less possibility for errors or issues when later modifying it and forgetting to change something. This line works as described here

a - '0' is equivalent to ((int)a) - ((int)'0'), which means the ascii values of the characters are subtracted from each other. Since 0 comes directly before 1 in the ascii table (and so on until 9), the difference between the two gives the number that the character a represents.

Since we are already storing the choices in an array, we don't really want to have that same information at another place as well - that would make it harder to update the program. That is why I refer to the choices array from the main function as well.

Also note that using namespace std; is fine in such a small program, but can get messy if you get confused about which functions are from which namespace.

The comment to your question about global variables points out that it would be cleaner to instead pass getUserChoice as parameter. And you only access getComputerChoice in one function, so it would be better to not use a global variable for that. That way, it is clear to a reader that there are no evil side effects.

Note that rand() is pseudorandom, and also it is the same on each run. Please see the link - I have not modified this in my version of your code.

---

Here is the complete program as I modified it:

#include <iostream>
#include <cstdlib>
#include <string>

using namespace std;

string choices[] = {"Rock", "Paper", "Scissors"};

void determineWinner(int getUserChoice)
{   

    //random computer choice
    int getComputerChoice = rand() % 3 + 1; // 1, 2, or 3

    //displays the users input choice
    if (getUserChoice == 4)
    {
        cout << "You have chosen to exit the game. The game will now end." << endl;
        return;
    }
    else if (getUserChoice > 4)
    {
        cout << "You have chosen something weird!" << endl;
        return;
    }
    else
    {
        cout << "You chose " << choices[getUserChoice-1] << "." << endl;
    }

    //display the computers input choice
    cout << "The computer chose " << choices[getComputerChoice-1] << "." << endl;

    //determines winner
    if ((getUserChoice + 1) % 3 == getComputerChoice % 3)
    {
        cout << "I'm sorry, the computer has beat you." << endl << endl;
    }
    else if ((getUserChoice + 2) % 3 == getComputerChoice % 3)
    {
        cout << "Congratulations, you win!" << endl << endl;
    }
    else
    {
        cout << "This game is tied. " << endl << endl;
    }   
}

int main()
{
    //displays a description of the game
    cout << "Rock, Paper, Scisscors Game!"  << endl << endl;
    cout << "You will be playing against the computer." << endl;
    cout << "Rock beats Scissors; Paper beats Rock; Scissors beats Paper." << endl << endl;
    cout << "------------------------------------------------------------------" << endl << endl;
    cout << "Please choose from the following" << endl;
    for (int i=1; i<4; i++)
    {
        cout << ((char)((int)'0'+i)) << ". " << choices[i-1] << endl;
    }
    cout << "4. Quit" << endl << endl;

    //loop sequence that allows player to play again if their input is not 4
    int getUserChoice;
    while (getUserChoice != 4)
    {
        // play begins
        char reply;
        cin >> reply;
        cout << endl;

        //assinging the numbers to getUserChoice
        getUserChoice = reply - '0';

        //calls the void function to display the winner
        determineWinner(getUserChoice);

    }

    return 0;
}

Answer 3

Nice start point.

I'd like to suggest the following in the spirit of splitting everything into smaller functions and making the code easier to read:

#include <iostream>
//#include <cstdlib> // We aren't using this so remove it
#include <string>

using namespace std; // avoid this for larger programmes

enum Options{
    Rock, Paper, Scissors, Quit
};

This is a lot cleaner.

Let's clean up main

int main()
{
    run_game();
    return 0;
}

Pretty obvious what main is doing now. No need for comments,

and this would lead us to

void run_game() {
    Options player_choice, cpu_choice;
    bool quit = false;
    while(quit == false) {
        player_choice = get_player_choice();
        if (player_choice == Quit) {
            quit = true;
            return;
        }
        cpu_choice = get_computer_choice();
        show_results(player_choice, cpu_choice);
    }
}

I've omitted the other functions.