"Rock Paper Scissors" game

Question

I have created a rock paper scissors game in C++.

#include <iostream>
#include <map>
#include <vector>
#include <time.h>

std::vector<std::string> gameOptions {"Rock", "Paper", "Scissors"};

// Map of Key as winner and Value as loser
std::map<std::string, std::string> gameRules {
    {"Rock", "Scissors"},
    {"Scissors", "Paper"},
    {"Paper", "Rock"}
};

std::string checkWinner(const std::string& playerSelection, const std::string& pcSelection)
{
    if (playerSelection == pcSelection) return "Draw!";
    if (gameRules[playerSelection] == pcSelection) return "Player Wins!";
    return "PC Wins!";
}

bool isUserInputValid(const std::string& playerSelection)
{
    for (std::string option : gameOptions)
    {
        if (playerSelection == option) return true;
    }
    return false;
}

void promptUserInput()
{
    std::cout << "Game Started! Enter your choice from below:" << std::endl;
    std::cout << "Rock" << std::endl;
    std::cout << "Paper" << std::endl;
    std::cout << "Scissors" << std::endl;
}

int main()
{
    promptUserInput();
    std::string playerSelection;
    std::cin >> playerSelection;

    while (!isUserInputValid(playerSelection))
    {
        promptUserInput();
        std::cin >> playerSelection;
    }

    std::string pcSelection;
    srand(time(NULL));
    int random = rand();
    pcSelection = gameOptions[random % 3];

    std::cout<< "The result is: " << checkWinner(playerSelection, pcSelection) << std::endl;
    std::cout<< "Player Selection:\t" << playerSelection << std::endl;
    std::cout<< "PC Selection:\t" << pcSelection << std::endl;
}

Are there any parts that can be improved on?

Answer 1

Overview

I don't like how you are simply passing strings around for the values. Its simple but not very optimal. I would create some enum types to represent the different states (Win, Lose, Draw) and/or (Rock, Paper, Scissors). You can then convert these to string values if you need to present them on a console like interface.

Code Review

Sure:

std::vector<std::string> gameOptions {"Rock", "Paper", "Scissors"};

But would be nice to be able to convert these to an enum for easy comparison.

---

std::map<std::string, std::string> gameRules {
    {"Rock", "Scissors"},
    {"Scissors", "Paper"},
    {"Paper", "Rock"}
};

Sure: But this representation can just as easily be represented by a switch. If you really want to represent the states and how they play against each other I would create an array that allows you to map to hands into a decision that wins/loses/draws.

I would do this:

enum State {Win, Lose, Draw};                                                                         
enum Move {Rock, Paper, Scissors};                                                                      
                                                                                                       
State challenge[3][3] = {{Draw,  Lose, Win  },
                         {Win,   Draw,  Lose}, 
                         {Lose, Win,   Draw }
                        };                      
                                                                                                       
int main()                                                                                             
{                                                                                                      
    Move    human = Rock;                                                                              
    Move    pc    = Scissors;                                                                           
                                                                                                       
    State   humanState = challenge[human][pc];                                                             
            
    // You can convert the state to a string by overloading
    // operator << for the type State.                                                                                           
    std::cout << humanState << "\n";                                                                   
}                                                                                                      

---

This is very expensive using strings.

std::string checkWinner(const std::string& playerSelection, const std::string& pcSelection)
{
    if (playerSelection == pcSelection) return "Draw!";
    if (gameRules[playerSelection] == pcSelection) return "Player Wins!";
    return "PC Wins!";
}

Much better using an enum. Then you could do a switch. Or you could do the challenge matrix I used above.

---

prefer to use standard algorithms rather than a manual loop:

bool isUserInputValid(const std::string& playerSelection)
{
    for (std::string option : gameOptions)
    {
        if (playerSelection == option) return true;
    }
    return false;
}

I would write it like this:

bool isUserInputValid(const std::string& playerSelection)
{
    return std::find(std::begin(option), std::end(option), playerSelection) != std::end(option);
}

---

I should not use std::endl when "\n" is enough. The difference is that std::endl will flush the output buffer (Which is not needed, the standard library will flush when it needs to).

This is the source of most speed concerns from new learners.

There is no need for multiple statements. You can use a single statement (with one string per line) or a single raw string.

void promptUserInput()
{
    std::cout << "Game Started! Enter your choice from below:" << std::endl;
    std::cout << "Rock" << std::endl;
    std::cout << "Paper" << std::endl;
    std::cout << "Scissors" << std::endl;
}

I would write like this:

void promptUserInput()
{
    std::cout << "Game Started! Enter your choice from below:\n"
              << "    Rock\n"
              << "    Paper\n"
              << "    Scissors\n";
}

Or with raw string literal.

void promptUserInput()
{
    std::cout << R"(
Game Started! Enter your choice from below:
Rock
Paper
Scissors
)";
}

---

Note: srand() should only be called once in an application.

    srand(time(NULL));
    int random = rand();

But really you should stop using this. It is after all a C library. There is a better C++ library. Example here.

Answer 2

Computing pcSelection can be factored out into a separate function.

If users enter an invalid choice, promptUserInput still outputs "Game Started! Enter your choice from below:". Users should instead be notified of an invalid choice they made.

The map gameRules could be renamed to something that conveys the fact that key defeats value (Instead of having it as comment).

Answer 3

The code looks good and well-organized. Here are a few suggestions:

• Use const wherever possible to make the code more readable and safe. For example, in checkWinner() and isUserInputValid(), the function parameters can be marked const.
• Use range-based for loop wherever possible to make the code more readable and safe. For example, in isUserInputValid(), the for loop can be replaced with a range-based for loop.
• Use enum instead of std::vector<std::string> for game options. This makes the code more readable and safer.
• Use auto wherever possible to make the code more readable and safer. For example, in checkWinner(), the return type can be replaced with auto.
• Avoid using using namespace std as it is considered bad practice.
• Add error handling to the code, for example, in case of invalid user input.