16
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ You might want to use switch instead of nested ifs. I tasked my students with writing R-P-S while having a better AI than just random in mind: this involved interfaces and decoupling players from move evaluation. \$\endgroup\$ – Oleg Lobachev Jul 28 at 9:24
  • \$\begingroup\$ Can you elaborate why the void function has to be there? I believe it is not a good style to have a void function the determine something. Something called "determineWinner" should usually return the winner. \$\endgroup\$ – Helena Jul 28 at 12:13
  • 1
    \$\begingroup\$ The indenting in the code is .... unconventional, and worse, inconsistent (e.g. braces at offset 0, 1, and 4. And 0, 1, 4, and 8 spaces indent for non-brace lines (0 near determineWinner();)). \$\endgroup\$ – Peter Mortensen Jul 28 at 21:48
  • 2
    \$\begingroup\$ You started off well by defining some constants (const int Rock = 1, Paper = 2, Scissors = 3, Exit = 4;) but when I look through the code I see magic numbers (1, 2, 3) everywhere -- why not use those constants?! \$\endgroup\$ – CompuChip Jul 29 at 9:19
  • \$\begingroup\$ @Kay you should use a text editor that manages indentation for you. If you don't know one, try visual studio code. \$\endgroup\$ – Relish Jul 29 at 18:20
28
\$\begingroup\$

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.

\$\endgroup\$
  • 4
    \$\begingroup\$ Nice explanation how to think about functions! It might be worthwile to note that endl and \n are only almost the same but flushing does not matter in this context. \$\endgroup\$ – lucidbrot Jul 27 at 16:44
  • 27
    \$\begingroup\$ That sounds like one counter-productive instructor \$\endgroup\$ – lucidbrot Jul 27 at 20:59
  • 5
    \$\begingroup\$ @Kay std::cout is the same as "using namespace std; cout" \$\endgroup\$ – Matthew Barclay Jul 28 at 0:44
  • 3
    \$\begingroup\$ I agree with your suggest, except for the last part n transforming the beats function. I believe the original way of using an explicit if condition is more readable and doesn't rely on properties of the underlying data representation. \$\endgroup\$ – Helena Jul 28 at 12:18
  • 2
    \$\begingroup\$ @Quuxplusone Why would -2 and -1… Oh. C's % operator is the remainder. I always get caught on that one. \$\endgroup\$ – wizzwizz4 Jul 28 at 17:41
5
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Your modulo math isn't quite right. if ((getUserChoice + 1) % 3 == getComputerChoice) will never be true if the computer picks 3 (scissors). \$\endgroup\$ – Quuxplusone Jul 27 at 16:33
  • 3
    \$\begingroup\$ You shouldn't keep the using namespace std; in your solution. That's the 1st sin in c++ programming. \$\endgroup\$ – πάντα ῥεῖ Jul 27 at 16:36
  • 1
    \$\begingroup\$ @lucidbrot It's a general issue, and shouldn't become a (default) behavior. \$\endgroup\$ – πάντα ῥεῖ Jul 27 at 16:49
  • 1
    \$\begingroup\$ @πάνταῥεῖ I have to keep the "using namespace std;" in my code as it is a class requirement. Thanks for all the wonderful comments though. \$\endgroup\$ – Kay Jul 27 at 20:53
  • 1
    \$\begingroup\$ No problem @Kay, I'm happy that the initial reaction you got on stackoverflow did not scare you away from asking \$\endgroup\$ – lucidbrot Jul 27 at 20:58
4
\$\begingroup\$

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.

\$\endgroup\$
  • 6
    \$\begingroup\$ Why not bool quit = false? Also, the return makes the whole variable redundant. And in addition, variables should be initialized at first use instead of declaring them at the very top of the function. \$\endgroup\$ – Roland Illig Jul 29 at 5:45
  • \$\begingroup\$ Totally agree. I'll edit the post for bool as that should have been there in the first place. Quit could also be done much better (I was originally going to return the quit flag but I didn't do that in the end), but my main focus was on making it easier to read. Definitely not the best example, but a step in the right direction. \$\endgroup\$ – David Jul 29 at 15:29
  • 1
    \$\begingroup\$ Why while (quit == false)? Isn't it clearer with while (!quit)? \$\endgroup\$ – md2perpe Jul 29 at 19:16
  • \$\begingroup\$ For most, yes. For a newer programmer, maybe not. \$\endgroup\$ – David Jul 30 at 3:13
  • 1
    \$\begingroup\$ Having all the comments, using the "namespace std", the bool function being like it is, these are all class requirements. I have tried using better and cleaner code and my teacher posted a forum saying that we are not allowed to read ahead or use Google/Youtube to help us. I don't completely agree with this since even the senior programmers use at least Google/Stack-overflow but I am just following the rules of the class! Thanks again for all the awesome suggestions for cleaning up my code! \$\endgroup\$ – Kay Aug 25 at 13:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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