Simple rock-paper-scissors game

Question

This is a simple rock-paper-scissors program implemented using C++. Any feedback or suggestions for improvement are welcome.

Code:

main.cpp:

#include <iostream>
#include <string>
#include "rock-paper-scissorss.h"
using namespace std;

void whoWon(string player_move, string computer_move){
    if(player_move == "rock"){
        if(computer_move == "paper")
            cout<<"You chose rock and the computer chose paper, the computer wins!"<<endl;

        else if(computer_move == "scissors")
            cout<<"You chose rock and the computer chose scissors, you win!"<<endl;

        else
            cout<<"You chose rock and the computer chose rock, it's a tie!"<<endl;
    }

    else if(player_move == "paper"){
        if(computer_move == "scissors")
            cout<<"You chose paper and the computer chose scissors, the computer wins!"<<endl;

        else if(computer_move == "rock")
            cout<<"You chose paper and the computer chose rock, you win!"<<endl;

        else
            cout<< "You chose paper and the computer chose paper, it's a tie!"<<endl;
    }

    else{
        if(computer_move == "rock")
            cout<<"You chose scissors and the computer chose rock, the computer wins!"<<endl;

        else if(computer_move == "paper")
            cout<<"You chose scissors and the computer chose paper, you win!"<<endl;

        else
            cout<<"You chose scissors and the computer chose scissors, it's a tie!"<<endl;
    }
}

int main(int argc, char** argv) {
  \
  char play_again = 'y';
  cout<<"Please enter your name: "<<endl;
  string name;
  getline(cin,name);
  cout<<"Hello "<<name<<", welcome to Rock-Paper-Scissors!"<<endl;

    Player player_1(name);
  while(play_again == 'y'){


    cout<<"Please enter the move you wish to make: "<<endl;
    string move;
    getline(cin,move);
    while(1){
        if((move == "rock") || (move == "paper") || (move == "scissors")){

            player_1.setMoveChoice(move);
            break;
        }

        else{
            cout<<"Invalid move choice! Please enter rock, paper, or scissors"<<endl;
            getline(cin,move);
        }
    } 


    Computer com_1;
    com_1.setMoveChoice();

   string p_move = player_1.getMoveChoice();
   string c_move = com_1.getMoveChoice();


   whoWon(p_move,c_move);



   cout<<"Would you like to play again?(y/n)"<<endl;

   cin>>play_again;
   cin.ignore();



    } 



 return 0;
}

rock-paper-scissors.cpp:

#include "rock-paper-scissorss.h"
#include <cstdlib>

Computer :: Computer(){
    num_generated = rand() % 3 + 1;
}

void Computer ::  setMoveChoice(){
   if(num_generated == 1)
       move_selected = "rock";

   else if(num_generated == 2)
       move_selected = "paper";

   else
       move_selected = "scissors";

}

string Computer :: getMoveChoice(){
    return move_selected;
}




Player :: Player(string name){
    player_name = name;

}

void Player :: setMoveChoice(string choice){
    move_selected = choice;
}

string Player :: getMoveChoice(){
    return move_selected;
}

rock-paper-scissorss.h:

#ifndef ROCK_PAPER_SCISSORSS_H
#define ROCK_PAPER_SCISSORSS_H

#include <iostream>
#include <string>
using namespace std;



class Computer{

private:
    int num_generated;
    string move_selected;

public:
    Computer();
    void setMoveChoice();
    string getMoveChoice();

};

class Player{

private:
    string move_selected;
    string player_name;

public:
    Player(string);
    void setMoveChoice(string move);
    string getMoveChoice();

};

#endif /* ROCK_PAPER_SCISSORSS_H */

Answer 1

Welcome to codereview. Here's a selection of suggestions.

num_generated = rand() % 3 + 1;

This is probably fine for this use case. Be aware that

1. rand() is generally agreed not to be random enough for many uses where you need a random number. Consider instead using the <random> header.
2. Using %3 to condense the spread of random numbers into 0 through 2 slightly biases the result. Consider what happens if rand() uniformly and randomly produced numbers between 0 and 4 and you used this approach: getting low values would be slightly more likely than getting high values.
3. rand() requires seeding with srand() or the results won't vary from run to run.
4. The + 1 goes from a range of 0 through 2 to a range of 1 through 3. That's not inherently a problem (it doesn't cause any issues with this program) but the very strong convention is that counting starts from 0. If, say, you had an array of moves and you used this random number to look up the array, one of the moves should be move 0.

Putting that in the constructor, specifically

Computer :: Computer(){
    num_generated = rand() % 3 + 1;
}

means that a given Computer object only ever produced one move. It would probably be better if you could reuse the same object.

public:
    Computer();
    void setMoveChoice();
    string getMoveChoice();
};

Consider setting up some sort of inheritance structure. If there was a base class Player which HumanPlayer and ComputerPlayer inherited from, it would be easier to write the program flexibly. You could have the same game logic, and swap in different sorts of players for PvP games, CvC simulations, and potentially new additions such as playing against another player on the internet.

move_selected = "rock";

For a problem like this, it's better to use integer (or better still enum) types. Keep strings for when the task is inherently about the text (including text input and output) rather than the thing the text represents. Among other things, strings are expensive to work with and tend to confuse the compiler in places where it would otherwise be able to help point out bugs.

On the topic of strings, there is a lot of redundancy in whoWon. Consider separating out the logic of working out who won from the logic of displaying it. Every output has the form.

cout<< "You chose " << player_move << " and the computer chose "<< computer_move <<", " << result <<endl;

A c++ specific issue:

using namespace std;

It is generally agreed that this should be avoided, especially in header files. Better to write std::string and such like in full. Otherwise there can be ambiguity (including in files that at some level include that header, which the programmer is not fully aware of) between functionality defined by the standard library and functionality defined by the programmer or third party libraries.

endl

C++ programmers generally prefer putting a newline (\n) into the string rather than using endl: endl both pushes a newline and forces the output buffer to flush. Only use it when you need the buffer to flush and the text to display immediately.

while(play_again == 'y'){

If you want to use a loop that always runs once, and then checks whether to run again at the end, consider a do { ... } while () loop instead. This avoids having to force the repeat condition before starting.

Answer 2

Read through and bookmark the C++ Standard Guidelines. Numbers I note later are citations from this.

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.)

---

void whoWon(string player_move, string computer_move){

Why are you passing strings by value? You are not going to alter them; you do not need a local copy of the string.

---

You have nested if/else for every combination of inputs. Your message strings are identical except for the choices being repeated.

Don’t duplicate code. Instead, abstract out what is different, and use data, parameters, helper functions, etc.

Functions should have a single responsibility. Your name whoWon sounds like it should determine the winner, but it never even returns anything.

For example, my Top-Down design would start out like this:

void play (string_view p1, string_view p2)
{
    const auto winner= RPS(p1,p2);
    cout << "You chose " << p1;
    cout << " and the computer chose " << p2;
    constexpr const char* announcement[3]= {
        "it's a tie!", "player wins!", "computer wins!" };
    cout << ", " << announcement[winner] << '\n';
}

See, the strings are only printed once. They are always the same except for the string value being inserted. So code it like that!

The winner code is chosen to serve well here. So, 1 for p1, 2 for p2, and 0 means tie. See that I used it as an index to the constant array, not a switch statement or anything like that.

Now the choosing of the winner is pushed down into another function. That is Top-Down Design — assume the details can/will be available and write just a high level description of the algorithm.

Now figure out how to determine the winner — only that, no printing or other effects — in an elegant manner.

Don’t just write out a brute force nest of if statements. At the very least, you doubled the amount of logic needed since the scoring is the same for each direction: Paper beats Scissors regardless of which player had which.

First I want to encode the choices as numeric values, rather than strings. You could have done this as part of the UI and passed enumerations all the way down, but that’s not the case. Maybe you’ll change it — anyway, I’ll leave that detail to the student and take it up from there.

enum RPS_t { Rock=0, Scissors=1, Paper=2 };

using winner_t = int;  // 0 for tie, player number matches parameter position

winner_t RPS (RPS_t hand1, RPS_t hand2)
{
// _Adjacent_ values will show a winner by its number.
// Non-adjacent can be considered modulo 3, so that Rock is also 3.
// but Rock-Paper is the only case to worry about, so just handle it directly.
const int difference = hand2 - hand1;
switch (difference) {
    1: return 2;
   -1: return 1;
    0: return 0;
   // Rock/Paper
   -2: return 2;  // hand1 had Paper
    2: return 2;  // hand2 had Paper
    }
}

Of course, check that over for mistakes.

---

string p_move = player_1.getMoveChoice();
string c_move = com_1.getMoveChoice();

It seems that the only reason for having classes is to provide different ways to choose the move. But the code does not use it in a polymorphic manner, so there’s no difference to simply having functions with different names, like getHumanMove and getComputerMove. You don’t need classes at all for this.

Furthermore, you do the human move in-line in main before calling this! Put all the promting and reading from the keyboard into the getHumanMove function instead.

---

Now it’s been pointed out by others that C’s legacy rand() function is rather bad. Here is what you use for coding now: #include <random>.

You want a uniform distribution of integers. So, copy the example from the page on uniform_int_distribution but change the distribution to (0,2).

When you turn the move choice into a string, you have a chain of if statements. You could have used a switch statement instead. But, you don’t need decision making at all — just a table of strings to choose from, like we used earlier.

constexpr const char* moves[3]= {"rock", "scissors", "paper"};

Now the random number n 0, 1, or 2 can be mapped to the name simply by stating moves[n].

You can use the same array to validate input from the user. Something like:

string in_human;
do {
    prompt;
    in_human = get_input;
    } while (in_human is not a member of moves);

That test can be done using one of the Standard Algorithms. std::find looks good, since it takes a range of values to search and an item to match, which is exactly what we have.

bool valid_move (string_view s)
{
    auto it = std::find(std::begin(moves), std::end(moves), s);
    // returns _end_ if not found.
    return it != std::end(moves);
}

Notice again the use of Top-Down Decomposition of the problem: First figure out the logic of looping until valid input is received. Then flesh out each step with its own code, possibly in a separate function. These are simpler with more narrow responsibility, so you have easier problems to work on now.

In general, a passage of code should be written at a single level of abstraction. Fiddly details should be pushed down, not exposed in the middle of high-level reasoning.

---

Things to work on

1. How do you pass string values around?

2. Sketch out the logic before you code. Think about Top-Down Decomposition of the problem.

3. When you get down to the level of a piece of logic with a single responsibility, and sit down to write that,

   • don’t just brute-force through it. Figure out a simple way to get the results.
   • don’t repeat code. Abstract the common parts and put the differences in data or caller parameters.

In any case, Keep it up!