3
\$\begingroup\$

As a beginner, this is my second game in C++. It is a very simple rock-paper-scissors game. User or computer wins the game after a given number of rock,paper or scissors matches. I'd appreciate any comment improving or criticising my code in any way. Thanks!

// Rock-Paper-Scissors game
#include <iostream>
#include <random>
#include <chrono>

char getUserInput();
char getCompInput();
void showValue(char x);
void getWinner(char x, char y);
int getScore(char x, char y);

int main()
{
    std::cout << "Welcome to Rock-Paper-Scissors Game!\n";

    // Ask how many matches to play
    int matches;
    std::cout << "How many games do you want to play?: \n";
    std::cin >> matches;

    // check if user enters a valid number
    while (matches < 1)
    {
        std::cout << "Please enter a valid number.\n";
        std::cout << "How many games do you want to play? \n";
        std::cin >> matches;
    }

    // Play the game and get the score
    int countmatches = 0;
    int score = 0;
    while (countmatches < matches)
    {
        char player = getUserInput();
        std::cout << "You chose: ";
        showValue(player);
        char comp = getCompInput();
        std::cout << "Computer chose: ";
        showValue(comp);
        getWinner(player, comp);
        score = score + getScore(player, comp);
        countmatches++;
    }

    std::cout << matches << " matches played in total. \n";

    // Declare the winner
    if (score < 0)
    {
        std::cout << "YOU LOST THE GAME! \n";
    }
    else if (score == 0)
    {
        std::cout << "IT'S A TIE! \n";
    }
    else
    {
        std::cout << "YOU WON THE GAME! \n";
    }

    return 0;
}

// ask for user input
char getUserInput()
{
    char userInput{' '};
    std::cout << "Rock, paper or scissors? (r-p-s): \n";
    std::cin >> userInput;
    if (userInput != 'r' && userInput != 'p' && userInput != 's')
    {
        std::cout << "Please enter a valid character. \n";
        std::cout << "Rock, paper or scissors? (r-p-s): \n";
        std::cin >> userInput;
    }
    return userInput;
}

// generate computer's choice using a random number generator
char getCompInput()
{
    std::mt19937 mt{static_cast<std::mt19937::result_type>(std::chrono::steady_clock::now().time_since_epoch().count())};
    std::uniform_int_distribution randDist{1, 3};
    int r = randDist(mt);

    switch (r)
    {
    case 1:
        return 'r';
        break;
    case 2:
        return 'p';
        break;
    case 3:
        return 's';
        break;
    }

    return 0;
}

// show choice of r, p or s
void showValue(char x)
{
    switch (x)
    {
    case 'r':
        std::cout << "Rock\n";
        break;
    case 'p':
        std::cout << "Paper\n";
        break;
    case 's':
        std::cout << "Scissors\n";
        break;
    }
}

// find the winner
void getWinner(char x, char y)
{
    if ((x == 'r' && y == 'p') || (x == 'p' && y == 's') || (x == 's' && y == 'r'))
    {
        std::cout << "YOU LOST!\n";
    }
    else if (x == y)
    {
        std::cout << "IT'S A TIE!\n";
    }
    else
    {
        std::cout << "YOU WON!\n";
    }
}

// count score
int getScore(char x, char y)
{
    if ((x == 'r' && y == 'p') || (x == 'p' && y == 's') || (x == 's' && y == 'r'))
    {
        return -1;
    }
    else if (x == y)
    {
        return 0;
    }
    else
    {
        return 1;
    }
}

\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

Overview

Overall OK.

Your main issue is failure to reset the stream state on failure and assuming your human follows instructions. A bit more work there.

I rather than use a char to hold the state. I would use an enum

enum Throw {Rock, Paper, Scissors};

This will make the code easier to read. Also this matches up to zero/one/two so it makes accessing arrays easier (see below).

Codereview

This looks like a C interface. Did you not want to write a C++ application?

char getUserInput();
char getCompInput();
void showValue(char x);
void getWinner(char x, char y);
int getScore(char x, char y);

When reading data from an untrusted source (always - but most specifically humans), you need to validate that the read worked.

    int matches;
    // This read assumes that the next character on the
    // input stream is a number. If it is not then the
    // read will fail (I am not sure if `matches` is set to zero
    // on a failure) but I would not make that assumption.
    // defensive coding and all.
    std::cin >> matches;

    // But even if a read failure set matches to zero.
    // A read failure will put the stream into an error state.
    // Any subsequent read operation will not work until you
    // get the stream out of the error state.
    while (matches < 1)

The correct way to do this is:

    int matches = 0;
    std::cout << "How many games do you want to play?: \n";

    do {
        if (std::cin >> matches) {
            // Read worked.
        }
        else {
            // If the failure was because of EOF.
            // Then you can't get any more input and it
            // simply fail again so exit.
            if (std::cin.eof()) {
                throw std::runtime_error("Bad EOF on input stream");
            }
            // Read failed we need to reset the state of the
            // stream before we do another read.
            std::cin.clear();
            // Since interactive user input is line based.
            // Let us ignore the rest of the line.
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
        if (matches < 1) {
            std::cout << "Please enter a valid number.\n";
            std::cout << "How many games do you want to play? \n";
        }
    }
    while (matches < 1);

You are making an assumption that if the user fails they will get it correct the second time. I would put that in a loop until they get it correct.

    if (userInput != 'r' && userInput != 'p' && userInput != 's')
    {
        std::cout << "Please enter a valid character. \n";
        std::cout << "Rock, paper or scissors? (r-p-s): \n";
        std::cin >> userInput;
    }

Initialize this only once. Then re-use.

    std::mt19937 mt{static_cast<std::mt19937::result_type>(std::chrono::steady_clock::now().time_since_epoch().count())};

This object is relatively expensive to create. Best to create once then reuse the same object. To do that make it a static object.

Also we have a random seed generator!

    static std::random_device  rd; 
    static std::mt19937        mt{rd()};

I think this is overly verbose.

    switch (r)
    {
    case 1:
        return 'r';
        break;
    case 2:
        return 'p';
        break;
    case 3:
        return 's';
        break;
    }

Also no need for a break after a return. I would write like this:

    // PS: Why 1->3 we are programmers.
    //     Numbers start at zero!
    switch (r)
    {
        case 1:  return 'r';
        case 2:  return 'p';
        case 3:  return 's';
    }

But I would use an array.

    static char result[] = {'r', 'p', 's'};
    return result[r-1];

Don't add code that will never be used.

    return 0;
}

Sure:

void showValue(char x)
{
    switch (x)
    {
    case 'r':
        std::cout << "Rock\n";
        break;
    case 'p':
        std::cout << "Paper\n";
        break;
    case 's':
        std::cout << "Scissors\n";
        break;
    }
}

Rather than a switch, I would again use a data structure.

static std::map<char, std::string> pritty {{'r', "Rock"}, {'p', "Paper"}, {'s', "Scissors"}};

std::cout << pritty[x] << "\n";

\$\endgroup\$
2
  • \$\begingroup\$ You have a likely infinite loop when std::cin.eof(). Need to check for that and bail out before calling clear(). \$\endgroup\$ Jan 11 at 9:39
  • 2
    \$\begingroup\$ @TobySpeight: I hate user input :-( \$\endgroup\$ Jan 11 at 20:03
3
\$\begingroup\$

Stream error checking

Consider the following transcript:

Welcome to Rock-Paper-Scissors Game!
How many games do you want to play?: 
two
Please enter a valid number.
How many games do you want to play? 
2
Please enter a valid number.
How many games do you want to play? 

Since when is 2 not a valid number? What's going wrong?

The issue is you are not checking and clearing the "fail" state of std::cin. When std::cin >> matches finds two on the input stream, it cannot convert the string "two" to an integer, so it enters the fail state, and ignores almost all stream operations from that point forward. You must detect and cin.clear() the failure state in order to use the input stream again.

BUT ... there is a catch. The two is still sitting on the input stream, unconsumed. You need to discard that, or the next cin >> matches will immediately put the stream back in the failure state. Use cin.ignore(...) to discard the bad input.

    while (std::cin.fail() || matches < 1)
    {
        if (std::cin.fail())
        {
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
        std::cout << "Please enter a valid number.\n";
        std::cout << "How many games do you want to play? \n";
        std::cin >> matches;
    }
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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