21
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ related: codereview.stackexchange.com/questions/213842/… \$\endgroup\$ – Edward Dec 8 '19 at 21:08
  • 1
    \$\begingroup\$ Try to use arrays and loops instead of "if this or this or this or this", since you have a lot. Use either nested switches or conditionals for your decideOutcome logic, or maybe try a matrix (2D array) approach, e.g. 0 is rock, 1 paper, 2 scissors, then have a 2D lookup array with the index of the corresponding outcome as the values/elements in the array. Also yeah, use a do while loop and get rid of the useless quit variable. \$\endgroup\$ – Andrew Dec 10 '19 at 21:39
34
\$\begingroup\$

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");
\$\endgroup\$
  • \$\begingroup\$ Thanks! Lots of C++ specific recommendations in this answer, which is exactly what I was looking for! :) \$\endgroup\$ – Jessica Dec 8 '19 at 21:32
  • \$\begingroup\$ You might also want to peruse github.com/isocpp/CppCoreGuidelines/blob/master/… \$\endgroup\$ – Edward Dec 8 '19 at 22:05
  • 2
    \$\begingroup\$ The input validation in various places can be simplified by half by normalizing the strings to either upper or lower case: input = /*convert to lower case*/; if(input == "rock" || input == "r" || input == "1") { /*...*/ } and return input == "no" || input == "n" || input == "0";. \$\endgroup\$ – Casey Dec 9 '19 at 1:20
  • \$\begingroup\$ @Casey good point. Another way to do it is to use a std::unordered_map to easily translate from input to token. \$\endgroup\$ – Edward Dec 9 '19 at 1:24
  • 2
    \$\begingroup\$ Actually for the game loop, I'd not use for, but do while, which would completely get rid of the variable quit. \$\endgroup\$ – celtschk Dec 10 '19 at 13:22
10
\$\begingroup\$

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)];
}
\$\endgroup\$
  • \$\begingroup\$ While I also often suggest using enum class, in this case it seems to me that it’s better as a plain enum for brevity and clarity. Otherwise one would have to use a static_cast to use it as an array index. \$\endgroup\$ – Edward Dec 9 '19 at 10:35
  • \$\begingroup\$ @Edward : you've got an example here : your choices array is properly tucked away in the getBotChoice function. The static_cast (or C style cast) would be present only once, not scattered at every call. enums or enum classes all the same will have drawbacks until C++ gets proper compile-time reflection on them. \$\endgroup\$ – Laurent LA RIZZA Dec 9 '19 at 10:54
  • \$\begingroup\$ I meant the part where a Choice is turned into text for output. I’m starting to think that perhaps a non-enum class might be a nicer option. What do you think? \$\endgroup\$ – Edward Dec 9 '19 at 11:02
  • \$\begingroup\$ @Edward : This is one of the yucky points of enums (scoped or unscoped). You can't (yet) introspect the number of declared enumerators, nor generate a dumb string from the enumerator name. There is a common technique of declaring an extra enumerator NB_ENUMS at the end (your enumhad better be scoped, or you'll get name clashes), and you don't use it in user code, only to declare appropriately-sized arrays and such. This limits the power of enums. There is no satisfactory solution yet, until we get proper compile-time reflection in C++. The C preprocessor might help there :) \$\endgroup\$ – Laurent LA RIZZA Dec 9 '19 at 11:09
  • \$\begingroup\$ To sum up, I think the solution with enums is fine, but you have to make concessions, until enums have full power : using enum for C++20, compile-time reflection for : the number of enumerators, the list of enumerators, the string_views on the enumerator names, and the automatic generation of to_string. \$\endgroup\$ – Laurent LA RIZZA Dec 9 '19 at 11:18
9
\$\begingroup\$

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.

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.