5
\$\begingroup\$

I have created a rock paper scissors game in C++.

#include <iostream>
#include <map>
#include <vector>
#include <time.h>

std::vector<std::string> gameOptions {"Rock", "Paper", "Scissors"};

// Map of Key as winner and Value as loser
std::map<std::string, std::string> gameRules {
    {"Rock", "Scissors"},
    {"Scissors", "Paper"},
    {"Paper", "Rock"}
};

std::string checkWinner(const std::string& playerSelection, const std::string& pcSelection)
{
    if (playerSelection == pcSelection) return "Draw!";
    if (gameRules[playerSelection] == pcSelection) return "Player Wins!";
    return "PC Wins!";
}

bool isUserInputValid(const std::string& playerSelection)
{
    for (std::string option : gameOptions)
    {
        if (playerSelection == option) return true;
    }
    return false;
}

void promptUserInput()
{
    std::cout << "Game Started! Enter your choice from below:" << std::endl;
    std::cout << "Rock" << std::endl;
    std::cout << "Paper" << std::endl;
    std::cout << "Scissors" << std::endl;
}

int main()
{
    promptUserInput();
    std::string playerSelection;
    std::cin >> playerSelection;

    while (!isUserInputValid(playerSelection))
    {
        promptUserInput();
        std::cin >> playerSelection;
    }

    std::string pcSelection;
    srand(time(NULL));
    int random = rand();
    pcSelection = gameOptions[random % 3];

    std::cout<< "The result is: " << checkWinner(playerSelection, pcSelection) << std::endl;
    std::cout<< "Player Selection:\t" << playerSelection << std::endl;
    std::cout<< "PC Selection:\t" << pcSelection << std::endl;
}

Are there any parts that can be improved on?

\$\endgroup\$

3 Answers 3

8
\$\begingroup\$

Overview

I don't like how you are simply passing strings around for the values. Its simple but not very optimal. I would create some enum types to represent the different states (Win, Lose, Draw) and/or (Rock, Paper, Scissors). You can then convert these to string values if you need to present them on a console like interface.

Code Review

Sure:

std::vector<std::string> gameOptions {"Rock", "Paper", "Scissors"};

But would be nice to be able to convert these to an enum for easy comparison.


std::map<std::string, std::string> gameRules {
    {"Rock", "Scissors"},
    {"Scissors", "Paper"},
    {"Paper", "Rock"}
};

Sure: But this representation can just as easily be represented by a switch. If you really want to represent the states and how they play against each other I would create an array that allows you to map to hands into a decision that wins/loses/draws.

I would do this:

enum State {Win, Lose, Draw};                                                                         
enum Move {Rock, Paper, Scissors};                                                                      
                                                                                                       
State challenge[3][3] = {{Draw,  Lose, Win  },
                         {Win,   Draw,  Lose}, 
                         {Lose, Win,   Draw }
                        };                      
                                                                                                       
int main()                                                                                             
{                                                                                                      
    Move    human = Rock;                                                                              
    Move    pc    = Scissors;                                                                           
                                                                                                       
    State   humanState = challenge[human][pc];                                                             
            
    // You can convert the state to a string by overloading
    // operator << for the type State.                                                                                           
    std::cout << humanState << "\n";                                                                   
}                                                                                                      

This is very expensive using strings.

std::string checkWinner(const std::string& playerSelection, const std::string& pcSelection)
{
    if (playerSelection == pcSelection) return "Draw!";
    if (gameRules[playerSelection] == pcSelection) return "Player Wins!";
    return "PC Wins!";
}

Much better using an enum. Then you could do a switch. Or you could do the challenge matrix I used above.


prefer to use standard algorithms rather than a manual loop:

bool isUserInputValid(const std::string& playerSelection)
{
    for (std::string option : gameOptions)
    {
        if (playerSelection == option) return true;
    }
    return false;
}

I would write it like this:

bool isUserInputValid(const std::string& playerSelection)
{
    return std::find(std::begin(option), std::end(option), playerSelection) != std::end(option);
}

I should not use std::endl when "\n" is enough. The difference is that std::endl will flush the output buffer (Which is not needed, the standard library will flush when it needs to).

This is the source of most speed concerns from new learners.

There is no need for multiple statements. You can use a single statement (with one string per line) or a single raw string.

void promptUserInput()
{
    std::cout << "Game Started! Enter your choice from below:" << std::endl;
    std::cout << "Rock" << std::endl;
    std::cout << "Paper" << std::endl;
    std::cout << "Scissors" << std::endl;
}

I would write like this:

void promptUserInput()
{
    std::cout << "Game Started! Enter your choice from below:\n"
              << "    Rock\n"
              << "    Paper\n"
              << "    Scissors\n";
}

Or with raw string literal.

void promptUserInput()
{
    std::cout << R"(
Game Started! Enter your choice from below:
Rock
Paper
Scissors
)";
}

Note: srand() should only be called once in an application.

    srand(time(NULL));
    int random = rand();

But really you should stop using this. It is after all a C library. There is a better C++ library. Example here.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Thanks Martin for the feedbacks, I have some follow up questions: 1. "This is very expensive using strings": Q: Is it because we need to compare both strings (2 array of chars)? As opposed to int comparison which is way more straight forward. 2. "prefer to use standard algorithms rather than a manual loop": Q: Is it because standard algorithms are usually more optimized compared to manual loops? Thanks! \$\endgroup\$ Nov 15, 2023 at 1:51
  • 1
    \$\begingroup\$ @kit.kat.code: also, generally if you don't need to iterate over the keys in order, use std::unordered_map (hash table) instead of std::map (red-black tree). For a 3-entry dictionary, I don't know which operator[] will be fastest. But either is a significant cost, not just the comparison. vs. a 2D array of (hopefully) 1-byte elements, 2D indexing with 2 integers is just some simple math, like 3 * x + y which x86 or ARM can do in a couple cheap instructions, vs. loading something and comparing to see where to load next (tree) or comparing a hash to check for a match. \$\endgroup\$ Nov 15, 2023 at 16:07
  • 1
    \$\begingroup\$ @kit.kat.code: Standard algorithms usually result in less code you write yourself, less room for bugs to creep in, like off-by-one errors. Gluing together known-good pieces makes it simpler for future readers of your code to see what it does. (Unless you had to jump through hoops in order to use standard algorithms if your problem doesn't fit any of them naturally. In that case do write your own loops.) \$\endgroup\$ Nov 15, 2023 at 16:10
  • 1
    \$\begingroup\$ 1. "This is very expensive using strings" Yes the time to check a string is linear on the length (also the string is probably not in a register so memory accesses (though cache illeviates this it is still more expensive). But also logically it is more intuative to have name specifically typed values. \$\endgroup\$ Nov 15, 2023 at 16:57
  • 2
    \$\begingroup\$ 2. "prefer to use standard algorithms rather than a manual loop":Is it because standard algorithms are usually more optimized. Though that is true. That is not the reason why. Its because you know the algorithm is correctly implemented. It also communicates its meaning without you have to decipher the code. \$\endgroup\$ Nov 15, 2023 at 16:58
7
\$\begingroup\$

Computing pcSelection can be factored out into a separate function.

If users enter an invalid choice, promptUserInput still outputs "Game Started! Enter your choice from below:". Users should instead be notified of an invalid choice they made.

The map gameRules could be renamed to something that conveys the fact that key defeats value (Instead of having it as comment).

\$\endgroup\$
1
  • \$\begingroup\$ Good catch, thanks for sharing Ajay. \$\endgroup\$ Nov 15, 2023 at 1:42
1
\$\begingroup\$

The code looks good and well-organized. Here are a few suggestions:

  • Use const wherever possible to make the code more readable and safe. For example, in checkWinner() and isUserInputValid(), the function parameters can be marked const.
  • Use range-based for loop wherever possible to make the code more readable and safe. For example, in isUserInputValid(), the for loop can be replaced with a range-based for loop.
  • Use enum instead of std::vector<std::string> for game options. This makes the code more readable and safer.
  • Use auto wherever possible to make the code more readable and safer. For example, in checkWinner(), the return type can be replaced with auto.
  • Avoid using using namespace std as it is considered bad practice.
  • Add error handling to the code, for example, in case of invalid user input.
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I'm confused by the bullet "Avoid using using namespace std as it is considered bad practice." Where do you see that in the OP's code? Are you saying that using namespace std; is bad practice, or that using namespace-qualified identifiers is bad practice? \$\endgroup\$ Nov 15, 2023 at 19:10
  • \$\begingroup\$ @ubuntubuntu Thanks for the points raised :) Can you advice on some questions that I wish to clarify: "Use const wherever possible to make the code more readable and safe." --> Is it because this is to prevent unexpected modifications to the variables? "Use range-based for loop wherever possible" --> Is this to prevent some sort of out of range issues? "Use auto wherever possible to make the code more readable and safer. " --> Can you kindly advice on the motivation of this idea? \$\endgroup\$ Nov 16, 2023 at 9:18

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.