Rock, Paper, Scissors in C++

Question

I have a fair few years experience with programming, and am currently working in a software development role, writing internal web applications in C#.

This is the first C++ code I have ever written, so I therefore don't know the conventions, code smells, do's/don't's of the language.

I decided to write an extremely basic Rock, Paper, Scissors implementation using the command line.

Please let me know if I can improve anything?

main.cpp

#include <iostream>
#include <stdlib.h>

using namespace std;

enum Outcome { user, bot, draw };
string outcomeMap[3] = {"You win!", "Bot wins!", "It was a draw!"};
enum Choice { rock, paper, scissors };
string choiceMap[3] = {"rock", "paper", "scissors"};

Choice getUserChoice() {
    cout << "Rock, paper or scissors? " << endl;

    string input;
    getline(cin, input);

    if(input == "rock" || input == "ROCK" || input == "1" || input == "r" || input == "R") {
        return rock;
    } else if(input == "paper" || input == "PAPER" || input == "2" || input == "p" || input == "P") {
        return paper;
    } else if(input == "scissors" || input == "SCISSORS" || input == "3" || input == "s" || input == "S") {
        return scissors;
    } else {
        throw invalid_argument("You must choose rock, paper or scissors.");
    }
}

Choice getBotChoice() {
    auto randomNumber = rand() % 3 + 1;

    switch(randomNumber) {
        case 1:
            return rock;
        case 2:
            return paper;
        case 3:
            return scissors;
        default:
            throw invalid_argument("Random number was generated outside of the given range.");
    }
}

Outcome decideOutcomeOfGame(Choice userChoice, Choice botChoice) {
    if (userChoice == botChoice) {
        return draw;
    } else if (userChoice == rock && botChoice == paper) {
        return bot;
    } else if (userChoice == rock && botChoice == scissors) {
        return user;
    } else if (userChoice == paper && botChoice == rock){
        return user;
    } else if (userChoice == paper && botChoice == scissors){
        return bot;
    } else if (userChoice == scissors && botChoice == rock){
        return bot;
    } else if (userChoice == scissors && botChoice == paper) {
        return user;
    }
}

bool shouldGameExit() {
    cout << "Quit?" << "\r\n";
    string input;
    getline(cin, input);
    if(input == "no" || input == "NO" || input == "n" || input == "N" || input == "0") {
        return false;
    } else {
        return true;
    }
}

void gameLoop() {
    auto quit = false;
    while(!quit) {
        auto userChoice = getUserChoice();
        auto botChoice = getBotChoice();

        cout << "You chose " << choiceMap[userChoice] << "\r\n";
        cout << "The bot chose " << choiceMap[botChoice] << "\r\n";
        cout << outcomeMap[decideOutcomeOfGame(userChoice, botChoice)] << "\r\n";

        quit = shouldGameExit();
    }
}

int main() {
    gameLoop();
    return 0;
}

Answer 1

It's a good start! Here are some things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers). In this particular case, I happen to think it's perfectly appropriate because it's a single short program that and not a header. Some people seem to think it should never be used under any circumstance, but my view is that it can be used as long as it is done responsibly and with full knowledge of the consequences.

Use <cstdlib> instead of <stdlib.h>

The difference between the two forms is that the former defines things within the std:: namespace versus into the global namespace. Language lawyers have lots of fun with this, but for daily use I'd recommend using <cstdlib>. See this SO question for details.

Use for loops rather than while loops where practical

Your gameloop routine can be simplified a bit by using a for loop rather than a while loop. here is the current code:

void gameLoop() {
    auto quit = false;
    while(!quit) {
        // other stuff
        quit = shouldGameExit();
    }
}

I'd recommend writing it as a for loop to do several things. First, it changes the scope of the quit variable to solely within the loop. Second, it makes it clear how the exit condition is set:

void gameLoop() {
    for (auto quit = false; !quit; quit = shouldGameExit()) {
        // other stuff
    }
}

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Check return values and handle errors

The code calls getline but never checks for error return values. If getline encounters a problem, it sets the failbit. It's easy to check for this in C++, because of operator overloading. That is, one could do this:

getline(cin, input)
if (cin) {  // if getline was OK
    ///
}

Think of the user

Generally speaking, it's not unusual for users to type in faulty input. For that reason, I think I would not throw an exception from with getUserChoice. Instead of abruptly aborting the user out of the program, a friendlier way to do it would be to give the user a chance to correct the input. Here's one way to rewrite that function:

Choice getUserChoice() {
    Choice userchoice;
    for (auto valid{false}; !valid; ) {
        cout << "Rock, paper or scissors? \n";
        string input;
        getline(cin, input);
        if (cin) {
            if(input == "rock" || input == "ROCK" || input == "1" || input == "r" || input == "R") {
                valid = true;
                userchoice = rock;

            } else if(input == "paper" || input == "PAPER" || input == "2" || input == "p" || input == "P") {
                valid = true;
                userchoice = paper;
            } else if(input == "scissors" || input == "SCISSORS" || input == "3" || input == "s" || input == "S") {
                valid = true;
                userchoice = scissors;
            } else {
                cout << "Sorry, I didn't understand \"" << input << "\"\n";
            }
        }
    }
    return userchoice;
}

Note that one could code a return instead of using the userchoice and valid variables, but I prefer having an easier-to-read program flow without having to hunt for return statements. You can decide for yourself which flavor you prefer.

Make sure all paths return a value

The decideOutcomeOfGame routine returns the outcome of the game. It's probable that all combinations are enumerated, but I'd prefer to make sure the function returns something every time. Here's how I'd write it.

Outcome decideOutcomeOfGame(Choice userChoice, Choice botChoice) {
    if (userChoice == botChoice) {
        return draw;
    } else if ((userChoice == rock && botChoice == paper)
              || (userChoice == paper && botChoice == scissors)
              || (userChoice == scissors && botChoice == rock)) {
        return bot;
    }
    return user;
}

Consider using a better random number generator

You are currently using

auto randomNumber = rand() % 3 + 1;

Normally I recommend the use of the C++11 std::uniform_int_distribution to replace the old-style rand() but since you're using C++17, you could use std::sample. This reduces the getBotChoice() function to this:

Choice getBotChoice() {
    constexpr static std::array<Choice,3> choices{ rock, paper, scissors };
    static auto rnd{std::mt19937{std::random_device{}()}};
    std::vector<Choice> botChoice;
    std::sample(std::begin(choices), std::end(choices), std::back_inserter(botChoice), 1, rnd);
    return botChoice.front();
}

There is now no need for the throw because the code will never generate an invalid return value.

Use std::string_view where practical

Rather than mutable strings, the global variables outcomeMap and choiceMap would be better expressed as a std::array of std::string_view. In other words, instead of this:

string outcomeMap[3] = {"You win!", "Bot wins!", "It was a draw!"};

Write this:

constexpr array<string_view, 3> outcomeMap{"You win!", "Bot wins!", "It was a draw!"};

Consider encapsulating into a namespace

It doesn't seem as though these functions are going to be useful without each other. Because they're so closely related (and related as well to the data), I'd recommend consolidating everything into a namespace.

Understand return 0 in main

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main. I advocate omitting it to reduce the clutter and let the compiler generate code; others prefer to write it explicitly. It's up to you to choose which style you prefer, but either way you should know about this compiler behavior.

Simplify your code

The code currently contains this construct:

if(input == "no" || input == "NO" || input == "n" || input == "N" || input == "0") {
        return false;
    } else {
        return true;
    }

As @Deduplicator correctly notes in a comment, better would be to simply return the value of the expression which is already bool:

return !(input == "no" || input == "NO" || input == "n" || input == "N" || input == "0");

Answer 2

In addition to Edward's answer.

Use enum class instead of enum

enum class Outcome { user, bot, draw };
enum class Choice { rock, paper, scissors };

This does two things:

• Inject the names into the enum class, rather than into the surrounding namespace. (You then have to write Outcome::user instead of user, until C++20's using enum is available)
• Remove any implicit conversion to/from the underlying type.

A better random generation alternative

You want equiprobable numbers between 0 and 2 inclusive, to be able to get one of Choices values. Improving on Edward's answer, this would be:

Choice getBotChoice() {
    constexpr static std::array<Choice,3> choices{ rock, paper, scissors };
    static auto rnd{std::mt19937{std::random_device{}()}};
    std::uniform_int_distribution<int> distribution(0,choices.size() - 1);
    return choices[distribution(rnd)];
}

Answer 3

One more suggestion to add on top of these already great answers.

In the vein of "being nicer to your users," instead of

if(input == "rock" || input == "ROCK" || input == "1" || input == "r" || input == "R") {

Consider:

std::transform(input.begin(), input.end(), input.begin(),
                   [](unsigned char c){ return std::tolower(c); });
if(input == "rock" || input == "r" || input == "1") {

You never use input for anything but those conditional checks and there's no real reason to disallow your users from entering Title Case.

Answer 4

In addition to the other answers. You say you have several years of experience. In this time you should have learnt to let your IDE format the code for you, to get consistent spacing between the if and the parenthesis. You sometimes wrote if( and sometimes if (.

What you did really well though is naming the things. That's usually much harder to get right than the amount of whitespace in the code.