1
\$\begingroup\$

I just started to learn C++ and I decided to make a Rock Paper Scissors game. It works fine but I would like to know how can I improve it further. In particular I couldn't find an efficient way to implement a play again feature. Thanks in advance.

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

using namespace std;

int main() {

srand(static_cast<unsigned int>(time(NULL)));

string userChoice;
int computerNumber = rand() % 3 + 1;
string computerChoice;
string userName;

if (computerNumber == 1) {
    computerChoice = "rock";
} else if (computerNumber == 2) {
    computerChoice = "paper";
} else {
    computerChoice = "scissors";
}

std::cout << "Welcome to rock, paper, scissors! What is your name?" << endl;
getline(cin, userName);
std::cout << "Hello " << userName << "! What do you choose?" << endl;
getline (cin,userChoice);

for (int i=0;i<userChoice.length();i++){ 
    userChoice[i]=tolower(userChoice[i]);
}

while (userChoice != "rock" && userChoice != "paper" && userChoice != "scissors") {
    std::cout << "Sorry! Didn't get that. Please enter again." << endl;
    getline(cin, userChoice);
    for (int i=0;i<userChoice.length();i++){
        userChoice[i]=tolower(userChoice[i]);
    }
}


std::cout << "Press enter to continue..." << endl;
std::cin.ignore();

if(computerChoice == "rock" && userChoice == "rock") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "rock" && userChoice == "scissors") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "paper" && userChoice == "paper") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "paper" && userChoice == "rock") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "scissors" && userChoice == "scissors") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "scissors" && userChoice == "paper") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else {
    std::cout << "Congrats! You won!" << endl;
}

return 0;
}
\$\endgroup\$
5
\$\begingroup\$

Indententation

Normally code is indented between { and }

int main()
{
    // Start here
}

Namespace

Never Do this.

using namespace std;

If you had read any other C++ review on this board it would have told you not to do this. See: Why is “using namespace std” in C++ considered bad practice?.

There is a reason that the standard libraries are called std and not standard. Its to make prefixing them easy.

Rand

Though your use of srand() and rand() is perfectly valid. This version of the random number generated is now obsolete. You should look up how to use the new random number functionality provided by C++11.

https://stackoverflow.com/a/14009667/14065
https://stackoverflow.com/a/19666713/14065

Note:

int computerNumber = rand() % 3 + 1;

This is slightly biased against 3. Assume RAND_MAX is 32768.

If you rolled 32768 times. Then your distribution would be.

1 10923/32768   
2 10923/32768
3 10922/32768  <- Notice this is one smaller.

Invalid:

} else {
    std::cout << "Congrats! You won!" << endl;
}

Do you really want to have an invalid choice. You have already enumerated all the valid choices. If you get to this point there is a serious error in your code. So you need to indicate this (so you can catch it in testing).

else
{
    throw std::runtime_error("Error");
}

Data Driven Programming

Rather than write code to decide the winner:

if(computerChoice == "rock" && userChoice == "rock") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "rock" && userChoice == "scissors") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "paper" && userChoice == "paper") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "paper" && userChoice == "rock") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else if (computerChoice == "scissors" && userChoice == "scissors") {
    std::cout << "It was a tie!" << endl;
} else if (computerChoice == "scissors" && userChoice == "paper") {
    std::cout << "The computer won! Better luck next time!" << endl;
} else {
    std::cout << "Congrats! You won!" << endl;
}

Use a data structure. Assume you translate the choices into a number for each user.

0 -> rock
1 -> paper
2 -> scissor

// This array decides if who is the winner.
// 0 => Draw.
// 1 => Player 1
// 2 => Player 2
int winner[3][3] = {{0, 2, 1},
                    {1, 0, 2},
                    {2, 1, 0}};


// Then to decide the winner.
int theWinner = winner[player1Choice][player2Choice];

Advice

In particular I couldn't find an efficient way to implement a play again feature.

To make things like retrying the code you need to start using functions so that you can logically compartmentalize your code.

int main()
{
    std::string name = getPlayerName();
    do
    {
        int player1 = getComputerMove();
        int player2 = getPlayerMove();
        displayWinner(player1, player2);
    }
    while(doesPlayerWantToContinue());
}
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the help! However whenever I try to create a function, I always get the error: "Control reaches end of non-void function". How could I fix this? \$\endgroup\$ – Nikhil Deorkar Oct 12 '16 at 16:37
  • 1
    \$\begingroup\$ This means you are function has a non void return type (ie an integer or something) and there is no return statement at the end of your function. \$\endgroup\$ – Martin York Oct 12 '16 at 16:48

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.