Simple Rock Paper Scissors

Question

I made a Rock Paper Scissors game. It works fine but I would like to know how can I improve it further.

#include <iostream>
#include <algorithm>
#include <string>
#include <random>
#include <array>
#include <map>
#include <limits>

namespace
{
    enum Winner { Tie, Player, Computer, WinnerCount };
    enum Items { Rock, Paper, Scissors, ItemsCount };

    template<typename T, std::size_t N>
    using Matrix = std::array<std::array<T, N>, N>;
}


std::string user_choice(const std::array<std::string, Items::ItemsCount>& data)
{
    std::string result;
    static auto comp = [&result](const std::string& str) { return str == result; };
    bool is_input_valid = false;

    do
    {
        std::cout << "Enter your choice, rock, paper, or scissors: ";
        std::getline(std::cin, result);

        is_input_valid = !(std::cin.fail() || std::none_of(data.begin(), data.end(), comp));

        if (!is_input_valid) {
            std::cout << "Not valid input.\n";
        }

        std::cin.clear();
        std::cin.ignore(std::cin.rdbuf()->in_avail());
    } while (!is_input_valid);

    return result;
}

bool play_again()
{
    char result;
    bool is_input_valid = false;

    do
    {
        std::cout << "Do you want to play again? [y/n]: ";
        std::cin >> result;

        result = std::toupper(result);

        is_input_valid = !(std::cin.fail() || (result != 'Y' && result != 'N'));

        if (!is_input_valid)
        {
            std::cout << "Not valid input.\n";
        }

        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    } while (!is_input_valid);

    return result == 'Y';
}

int main()
{
    // Set up game
    const Matrix<Winner, Winner::WinnerCount> table
    {
        {  // Rock      Paper      Scissors
            { Tie,      Computer,  Player   }, // Rock
            { Player,   Tie,       Computer }, // Paper
            { Computer, Player,    Tie      }, // Scissors
        }
    };

    const std::array<std::string, Items::ItemsCount> data
    {
        "rock",
        "paper",
        "scissors"
    };

    std::map<std::string, Items> binding_items
    {
       { data[Items::Rock],      Items::Rock     },
       { data[Items::Paper],     Items::Paper    },
       { data[Items::Scissors],  Items::Scissors },
    };

    const std::array<std::string, Winner::WinnerCount> winner_result
    {
       "It's a tie!\n\n",
       "You win!\n\n",
       "You lose!\n\n"
    };

    std::mt19937 engine{ std::random_device()() };
    auto dist = std::uniform_int_distribution<>(0, Items::ItemsCount - 1);

    // Game loop
    do
    {
        std::cout << "\n\n\tRock Paper Scissors Game\n\n";

        const auto user_chose = user_choice(data);
        const Items player = binding_items[user_chose];

        const Items computer = static_cast<Items>(dist(engine));
        const auto computer_chose = data[computer];

        std::cout << "\n\nYou entered: " << user_chose << '\n';
        std::cout << "Computer chose: " << computer_chose << '\n';

        std::cout << winner_result[table[player][computer]];
    } while (play_again());
}

Answer 1

Looks pretty good at first glance! I'd say your main problem is "#define ONE 1" — you've gone out of your way to parameterize things that can't actually be changed in real life without massive changes elsewhere in the game logic. For example, you've set

enum Items { Rock, Paper, Scissors, ItemsCount };

which looks awesome... until someone decides to actually use that flexibility by changing it to

enum Items { Rock, Paper, Scissors, Lizard, Spock, ItemsCount };

and suddenly nothing compiles anymore. (They also have to change the local variables table and data and binding_item inside main, and the text of the cout inside user_choice, and maybe some more stuff too. By the way: variable names are important! data and table are not good names!)

The over-parameterization antipattern tends to travel along with the std::array antipattern — see "The 'array size constant' antipattern" (2020-08-06). Somehow, people who've caught the "named constants > magic numbers" virus also tend to have caught the "std::array > core language" virus as well. Where you write

template<typename T, std::size_t N>
using Matrix = std::array<std::array<T, N>, N>;

const Matrix<Winner, Winner::WinnerCount> table
{
    {  // Rock      Paper      Scissors
        { Tie,      Computer,  Player   }, // Rock
        { Player,   Tie,       Computer }, // Paper
        { Computer, Player,    Tie      }, // Scissors
    }
};

an expert C++ programmer would probably just write

Winner table[3][3] = {
    // Rock      Paper      Scissors
    {  Tie,      Computer,  Player   }, // Rock
    {  Player,   Tie,       Computer }, // Paper
    {  Computer, Player,    Tie      }, // Scissors        
};

Incidentally, did you notice your bug above? You dimensioned your table by WinnerCount, but you index into it using values of Item. You're simply getting lucky that WinnerCount (win, lose, tie) happens to be the same as ItemCount (rock, paper, scissors).

---

Be aware that

std::mt19937 engine{ std::random_device()() };

is a relatively low-entropy way to initialize a Mersenne Twister: it has 19937 bits of state, and you're seeding it with only 32 random bits. But C++ doesn't offer any concise way to seed a PRNG properly, so actually what you've got is the state of the art.

---

const Items computer = static_cast<Items>(dist(engine));

Throughout, you use plural words where singular would be more appropriate, and relatively abstract nouns (data, table, computer) where more concrete ones would be appropriate. I'd write:

Item computers_choice = static_cast<Item>(dist(engine));

indicating that what we've got now is not a computer but rather the computer player's choice of what to throw; and it is not multiple Items but rather a single Item. (I'd probably rename Item to Gesture, too.)

---

Your user_choice function returns std::string, which you then have to parse again in main. It should just return the Item it extracted, directly, instead of the string.

Also, the logic around the is_input_valid boolean seems pretty baroque. Think about how the code could be simplified by using a return or break statement inside the loop.

if (std::getline(std::cin, result)) {
    if (map_of_strings_to_gestures.contains(result)) {
        return map_of_strings_to_gestures.at(result);
    }
}
std::cout << "Not valid input.\n";
// and go around again

Also, try hitting Ctrl-D (or Ctrl-Z on Windows) at this prompt. That'll close stdin, which makes cin.fail() true, which means the input isn't valid... and then your code just goes into an infinite loop, doesn't it? I think you actually want to break out of the loop when you see EOF or an error — not stay in the loop!

---

For configuring the gestures, I think you should investigate X-macros. Here's my take: https://godbolt.org/z/8qhdcPWT5 The entirety of the "configuration" for the game is expressed in a single table at the top of the code:

#define GESTURES \
    X(Rock,     "rock",     BEATS(Scissors)) \
    X(Scissors, "scissors", BEATS(Paper)) \
    X(Paper,    "paper",    BEATS(Rock))

and I've also tested it with this configuration:

#define GESTURES \
    X(Rock,     "rock",     BEATS(Scissors) BEATS(Lizard)) \
    X(Scissors, "scissors", BEATS(Paper) BEATS(Lizard)) \
    X(Paper,    "paper",    BEATS(Rock) BEATS(Spock)) \
    X(Lizard,   "lizard",   BEATS(Spock) BEATS(Paper)) \
    X(Spock,    "spock",    BEATS(Scissors) BEATS(Rock))

(My version also eliminates the dependencies on <map>, <algorithm>, and of course <limits>.)

---

An obvious "next step" for this program would be to refactor it so that you can print a customized verb for each outcome:

Enter your choice (rock, scissors, paper): rock
You entered: rock
Computer chose: paper
Paper covers rock — you lose!

Enter your choice (rock, scissors, paper): scissors
You entered: scissors
Computer chose: paper
Scissors cuts paper — you win!

and so on.