Простые ножницы для бумаги Rock Paper

Я сделал игру «Камень, ножницы, бумага». Он работает нормально, но я хотел бы знать, как я могу его улучшить.

#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!nn",
       "You win!nn",
       "You lose!nn"
    };

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

    // Game loop
    do
    {
        std::cout << "nntRock Paper Scissors Gamenn";

        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 << "nnYou entered: " << user_chose << 'n';
        std::cout << "Computer chose: " << computer_chose << 'n';

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

На первый взгляд выглядит неплохо! Я бы сказал, ваша главная проблема «#define ONE 1«- вы изо всех сил пытались параметризовать вещи, которые на самом деле нельзя изменить в реальной жизни без серьезных изменений в других частях игровой логики. Например, вы установили

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

что выглядит потрясающе … пока кто-то не решит использовать эту гибкость, изменив ее на

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

и вдруг ничего больше не компилируется. (Они также должны изменить локальные переменные table и data и binding_item внутри main, а текст cout внутри user_choice, и, возможно, еще кое-что. Кстати: имена переменных важны! data и table не хорошие имена!)

Антипаттерн избыточной параметризации имеет тенденцию перемещаться вместе с std::array антипаттерн — см. «Антипаттерн ‘константа размера массива’» (2020-08-06). Каким-то образом люди, поймавшие вирус «именованные константы> магические числа», также, как правило, заразились вирусом «std :: array> core language». Где ты пишешь

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
    }
};

опытный программист на C ++, вероятно, просто напишет

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

Кстати, вы заметили свой баг выше? Вы измерили свой table по WinnerCount, но вы индексируете его, используя значения Item. Тебе просто повезло, что WinnerCount (победа, поражение, ничья) совпадает с ItemCount (камень ножницы Бумага).

Быть в курсе, что

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

— это относительно низкоэнтропийный способ инициализации Mersenne Twister: он имеет 19937 бит состояния, и вы заполняете его только 32 случайными битами. Но C ++ не предлагает краткого способа правильного заполнения ГПСЧ, так что на самом деле то, что у вас есть является современное состояние.

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

Повсюду вы используете слова во множественном числе там, где единственное было бы более уместным, и относительно абстрактные существительные (data, table, computer) там, где уместны более конкретные. Я бы написал:

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

что указывает на то, что у нас сейчас не компьютер, а компьютерный плеер. выбор из чего бросить; и это не несколько предметов, а один предмет. (Я бы, наверное, переименовал Item к Gesture, тоже.)

Ваш user_choice функция возвращает std::string, который вам затем нужно проанализировать очередной раз в main. Он должен просто вернуть Item он извлекается напрямую, вместо строки.

Кроме того, логика вокруг is_input_valid логическое значение кажется довольно барочным. Подумайте, как можно упростить код, используя return или же break оператор внутри цикла.

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

Также попробуйте нажать Ctrl-D (или Ctrl-Z в Windows) в этом запросе. Это закроет stdin, что делает cin.fail() true, что означает, что ввод недействителен … а затем ваш код просто переходит в бесконечный цикл, не так ли? Я думаю, ты действительно хочешь прорыв цикла, когда вы видите EOF или ошибку — не оставайтесь в цикле!

Для настройки жестов, я думаю, вам следует изучить X-макросы. Вот мой вывод: https://godbolt.org/z/8qhdcPWT5 Вся «конфигурация» игры выражена в единственной таблице в верхней части кода:

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

и я также протестировал его с этой конфигурацией:

#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))

(Моя версия также устраняет зависимости от <map>, <algorithm>, и конечно <limits>.)

Очевидным «следующим шагом» для этой программы будет ее рефакторинг, чтобы вы могли распечатать индивидуальный глагол для каждого результата:

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!

и так далее.