Rock-paper-scissors Game in C++

Question

Below you will find my first attempt at a C++ implementation of the classic game Rock-paper-scissors. If someone could look it over and point out or suggest any things that I could change to improve it, that would be brilliant and I would greatly appreciate it.

Player.h

#ifndef PLAYER_H
#define PLAYER_H
#include <iostream>
#include <string>
#include <memory>
#include <random>
class Player
{
    private:
        std::string name;
        std::string choice;

    public:
        Player(std::string Name);
        void setName(std::string Name);
        std::string getName() const;
        void setChoice(std::string c);
        std::string makeChoice();
        std::string getChoice() const;
};
#endif

Player.cpp

#include "Player.h"

Player::Player(std::string Name)
{
    name=Name;
    choice=" ";
}

void Player::setName(std::string Name)
{
    name=Name;
}

std::string Player::getName() const
{
    return name;
}

std::string Player::makeChoice()
{
    std::random_device rd;
    std::mt19937 gen(rd());
    std::uniform_int_distribution<> dis(1, 3);

    int playerChoice=dis(gen);

    switch(playerChoice)
    {
        case 1:
            choice="rock";
            break;
        case 2:
            choice="paper";
            break;
        case 3:
            choice="scissors";
            break;
        default:
            std::cout << "Please make a valid choice" << std::endl;
            break;
    }
    return choice;
}

void Player::setChoice(std::string c)
{
    choice=c;
}

std::string Player::getChoice() const
{
    return choice;
}

Main.cpp

#include "Player.cpp"
#include <iostream>
#include <memory>
#include <thread>
#include <chrono>

using namespace std;

void determineWinner(unique_ptr<Player> const & p1, unique_ptr<Player> const & p2)
{
    if(p1->getChoice()=="rock" && p2->getChoice()=="scissors")
    {
        cout << p1->getName() << " wins!" << endl;
    }

    else if(p1->getChoice()=="scissors" && p2->getChoice()=="rock")
    {
        cout << p2->getName() << " wins!" << endl;
    }

    else if(p1->getChoice()=="paper" && p2->getChoice()=="rock")
    {
        cout << p1->getName() << " wins!" << endl;
    }

    else if(p1->getChoice()=="rock" && p2->getChoice()=="paper")
    {
        cout << p2->getName() << " wins!" << endl;
    }

    else if(p1->getChoice()=="scissors" && p2->getChoice()=="paper")
    {
        cout << p1->getName() << " wins!" << endl;
    }

    else if(p1->getChoice()=="paper" && p2->getChoice()=="scissors")
    {
        cout << p2->getName() << " wins!" << endl;
    }

    else
    {
        cout << "It's a tie!" << endl;
    }
}

void playRockPaperScissors2(unique_ptr<Player> const & p1, unique_ptr<Player> const & p2)
{
    char player1Choice;
    char player2Choice;

    cout << "Player 1, please make your choice:" << endl;
    cout << "Press (r) for rock, (p) for paper, or (s) for scissors" << endl;
    cin >> player1Choice;

    switch(player1Choice)
    {
        case 'r':
            p1->setChoice("rock");
            break;
        case 'p':
            p1->setChoice("paper");
            break;
        case 's':
            p1->setChoice("scissors");
            break;
        default:
            cout << "Please make a valid choice" << endl;
            break;
    }

    cout << "Player 2, please make your choice:" << endl;
    cout << "Press (r) for rock, (p) for paper, or (s) for scissors" << endl;
    cin >> player2Choice;

    switch(player2Choice)
    {
        case 'r':
            p2->setChoice("rock");
            break;
        case 'p':
            p2->setChoice("paper");
            break;
        case 's':
            p2->setChoice("scissors");
            break;
        default:
            cout << "Please make a valid choice" << endl;
            break;
    }

    cout << "Ready, set" << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "rock..." << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "paper..." << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "scissors..." << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "shoot..." << endl;

    cout << p1->getName() << ": " << p1->getChoice() << endl;
    cout << p2->getName() << ": " << p2->getChoice() << endl;

    determineWinner(p1,p2);
}

void playRockPaperScissors(unique_ptr<Player> const & p1, unique_ptr<Player> const & p2)
{
    //Simulates players choosing either rock, paper, or scissors
    string player1Choice=p1->makeChoice();
    string player2Choice=p2->makeChoice();

    p1->setChoice(player1Choice);
    p2->setChoice(player2Choice);

    cout << "Ready, set" << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "rock..." << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "paper..." << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "scissors..." << endl;
    this_thread::sleep_for(chrono::seconds(1));

    cout << "shoot..." << endl;

    cout << p1->getName() << ": " << p1->getChoice() << endl;
    cout << p2->getName() << ": " << p2->getChoice() << endl;

    determineWinner(p1,p2);
}

int main()
{
    unique_ptr<Player> player1{new Player("Player1")};
    unique_ptr<Player> player2{new Player("Player2")};

    char modeChoice;

    cout << "Welcome to this simulation of the classic game Rock, Paper, Scissors!" << endl;
    cout << "There are two modes to choose from:" << endl;
    cout << "Press 1 for the completely computer simulated version or press 2 if you and a friend want to square off!" << endl;
    cout << "Or, if you've played this game too many times and are sick of it, press (q) to quit" << endl;
    cin >> modeChoice;

    switch(modeChoice)
    {
        case '1':
            playRockPaperScissors(player1,player2);
            break;
        case '2':
            playRockPaperScissors2(player1,player2);
            break;
        case 'q':
            cout << "Program terminating..." << endl;
            break;
        default:
            cout << "Please make a valid choice" << endl;
            break;
    }
}

Answer 1

You have all the game logic in a single function - playRockPaperScissors2(). If you extracted the logic, you could easily reuse sections of it, perhaps for other games, perhaps to make a computer/human version.

cout << "Player 1, please make your choice:" << endl;
cout << "Press (r) for rock, (p) for paper, or (s) for scissors" << endl;
cin >> player1Choice;

switch(player1Choice)
{
    case 'r':
        p1->setChoice("rock");
        break;
    case 'p':
        p1->setChoice("paper");
        break;
    case 's':
        p1->setChoice("scissors");
        break;
    default:
        cout << "Please make a valid choice" << endl;
        break;
}

This section in particular is essentially duplicated. You can create a generic function like this:

std::string RPSPlayerChoice() {
    cout << "Please make your choice:" << endl;
    cout << "Press (r) for rock, (p) for paper, or (s) for scissors" << endl;
    cin >> playerChoice;

    do {
        switch(playerChoice)
        {
            case 'r':
                return "rock";
            case 'p':
                return "paper";
            case 's':
                return "scissors";
            default:
                cout << "Please make a valid choice" << endl;
                break;
        }
    while (true);
}

If you really want to specify player turn, you could adjust it like this:

std::string RPSPlayerChoice(int turn) {
    cout << "Player " << turn << ", please make your choice:" << std::endl;
    cout << "Press (r) for rock, (p) for paper, or (s) for scissors" << std::endl;
    cin >> playerChoice;

    do {
        switch(playerChoice)
        {
            case 'r':
                return "rock";
            case 'p':
                return "paper";
            case 's':
                return "scissors";
            default:
                cout << "Please make a valid choice" << std::endl;
                break;
        }
    while (true);
}

You could easily adjust this to be a string name, or any type you want that has an output operator.

One other thing I see is you always use std::endl. std::endl flushes the output stream, which can make output unnecessarily expensive. It is better to force a new line with the '\n' new line character, and only flush once when you are done outputting:

cout << "Player " << turn << ", please make your choice:\n";
cout << "Press (r) for rock, (p) for paper, or (s) for scissors" << std::endl;

Answer 2

Some additional tips, mostly focused on performance and maintainability.

• In your player.h file you are including multiple headers that you don't really use in that header, such as <iostream> and <random>. Moving these to the cpp file can save you some compile time for larger projects. If you're only using pointers or references in the header, you can pre-declare them as well.
• Function parameters that are large (such as strings, vectors, lists, trees ...) and that you do not modify, are best passed as const references. Currently the function setName(std::string Name) will call the copy constructor for this string, while setName(const std::string& Name) would not.

• Use initializer lists where possible. With your current constructor of the Player class, an empty string for Name and Choice will be constructed and then the assignment operator for string will be called. Using the following code you construct the object in a single call.

  Player::Player(const std::string& Name) : name(Name), choice(" ") {}

• (This one is optional as it is personal preference) Generally speaking I like to mark my member variables as such. E.g. m_Name, m_Choice.
• For large return types, such as in getName, feel free to return a const reference. Now you're always returning a copy, which is quite a waste if all you're doing is reading from it.
• String comparisons are more expensive to do than integer or enum compares. So instead of storing the choice in a string, why not use an enum?
• You're allocating two new player objects using new. But looking at there scope there is no need for using new. Just allocate them on the stack using Player pl1("Player 1");.
• The function determineWinner is going to become troublesome when implementing something like rock-paper-scissors-lizard-Spock edition. Try to solve such a problem with maths rather than brute force. Wikipedia provides an excellent example:

The majority of such proposed generalizations are isomorphic to a simple game of modulo arithmetic, where half the differences are wins for player one. For instance, rock-paper-scissors-Spock-lizard (note the different order of the last two moves) may be modeled as a game in which each player picks a number from one to five. Subtract the number chosen by player two from the number chosen by player one, and then take the remainder modulo 5 of the result. Player one is the victor if the difference is one or three, and player two is the victor if the difference is two or four. If the difference is zero, the game is a tie.

The comments from the other answers are valid as well - try to avoid copy pasting. If you ever have to copy paste something try to think if it can't be made into a function, macro, class, ...