Rock–paper–scissors console game

Question

I wrote this console rock-paper-scissors game. I'm new to C++. I'd appreciate any suggestions to improve the code below. Thanks.

RPSCore.h

#ifndef RPS_CORE_H
#define RPS_CORE_H


class RPSCore
{
public:
    static int rpsWinner(char& playerChoice, char& computerChoice);
    static char getComputerChoice();
};

#endif // !RPS_CORE_H

RPSCore.cc

#include <random>
#include <chrono>
#include <cstdint>
#include <map>
#include <sstream>
#include <string>
#include <algorithm>

#include "RPSCore.h"

// private helper function
char getValueIfKeyExists(std::map<std::string, char>& dict, std::string& key)
{
    char ret = '\0';
    for (std::map<std::string, char>::iterator it = dict.begin(); it != dict.end(); it++)
    {
        if (it->first.compare(key) == 0)
        {
            ret = it->second;
            break;
        }
    }
    return ret;
}

int RPSCore::rpsWinner(char& playerChoice, char& computerChoice)
{
    std::map<std::string, char> rules = { {"rs", 'r'}, {"rp", 'p'}, {"sp", 's'} };
    std::stringstream ss;
    ss << playerChoice << computerChoice;
    std::string combination = ss.str();
    std::string combinationCopy{ combination };
    std::reverse(combinationCopy.begin(), combinationCopy.end());
    char p = getValueIfKeyExists(rules, combination) == '\0' ? getValueIfKeyExists(rules, combinationCopy) : getValueIfKeyExists(rules, combination);
    return p == playerChoice && p != computerChoice ? 1 : p == computerChoice && p != playerChoice ? 2 : -1; // 1-> player wins, 2-> computer wins, -1-> draw
}

char RPSCore::getComputerChoice()
{
    std::string options = "rps";
    uint32_t seed = static_cast<uint32_t>(std::chrono::system_clock::now().time_since_epoch().count());
    std::minstd_rand0 range(seed);
    std::uniform_int_distribution<std::minstd_rand0::result_type> dist(0, 2);
    return options[dist(range)];    
}

main.cc

#include <iostream>
#include <string>
#include <cctype>

#include "RPSCore.h"

std::string input(std::string prompt="")
{
    std::string temp;
    std::cout << prompt;
    std::getline(std::cin, temp);
    return temp;
}

int main(int argc, char* argv[])
{
    char playerChoice = []() {
        std::string ret = "";
        do
        {
            ret = input("Your weapon >>> ");
            if (std::tolower(ret[0]) == 'r' || std::tolower(ret[0]) == 'p' || std::tolower(ret[0]) == 's') break;
        } while (1);
        return ret[0];
    }();
    char computerChoice = RPSCore::getComputerChoice();
    int winner = RPSCore::rpsWinner(playerChoice, computerChoice);
    std::cout << "Your choice: " << (playerChoice == 'r' ? "Rock" : playerChoice == 'p' ? "Paper" : "Scissors")  << " Computer's choice: " << (computerChoice == 'r' ? "Rock" : computerChoice == 'p' ? "Paper" : "Scissors") << " And " << (winner == 1 ? "You WIN!!!" : winner == 2 ? "Computer Wins :(" : "It's a draw :)");
    std::cin.get();
    return 0;
}

Answer 1

I'm thinking you named ret as in short for return. Don't shorten variable names. Also try to give varaible names that are meaningful. Naming it return because you return it is silly.

char ret = '\0';

Same here:

std::string temp;

This line looks very odd and is a sign of poorly named variables:

ret = it->second;

r, p s these are a good example of when you should be using an ENUM.

Try to avoid 1 letter variable names.

You can use const when variables should never change. For example: rules.

I suggest putting the condition inside the while instead. For example:

} while (std::tolower(ret[0]) != 'r' || std::tolower(ret[0]) != 'p' || std::tolower(ret[0]) != 's');

If you had an ENUM, you could check the entered value against all the values in the ENUM. This would look much better, and be easier to maintain. (E.g adding Lizard Spock)

The ENUM would also help for printing the values. (No more if r print rock etc)

Answer 2

I don't understand why you would do this:

char playerChoice = []() {
    std::string ret = "";
    do
    {
        ret = input("Your weapon >>> ");
        if (std::tolower(ret[0]) == 'r' || std::tolower(ret[0]) == 'p' || std::tolower(ret[0]) == 's') break;
    } while (1);
    return ret[0];
}();

Creating an inline function and calling it!

That could should be in its own named function.

char getPlayerChoice()
{
    std::string ret = "";
    do
    {
        ret = input("Your weapon >>> ");
        if (std::tolower(ret[0]) == 'r' || std::tolower(ret[0]) == 'p' || std::tolower(ret[0]) == 's') break;
    } while (1);
    return ret[0];
}

Then simply call it like normal.

char playerChoice = getPlayerChoice();

---

This line is excessively long:

std::cout << "Your choice: " << (playerChoice == 'r' ? "Rock" : playerChoice == 'p' ? "Paper" : "Scissors")  << " Computer's choice: " << (computerChoice == 'r' ? "Rock" : computerChoice == 'p' ? "Paper" : "Scissors") << " And " << (winner == 1 ? "You WIN!!!" : winner == 2 ? "Computer Wins :(" : "It's a draw :)");

Make it readable:

std::cout << "Your choice: "
          << (playerChoice == 'r' ? "Rock" : playerChoice == 'p' ? "Paper" : "Scissors")
          << " Computer's choice: "
          << (computerChoice == 'r' ? "Rock" : computerChoice == 'p' ? "Paper" : "Scissors")
          << " And "
          << (winner == 1
                  ? "You WIN!!!" 
                  : winner == 2 
                        ? "Computer Wins 
                        :(" : "It's a draw :)");

Would be even better if you used some named functions:

std::cout << "Your choice: " << choiceToOutput(playerChoice)
          << " Computer's choice: " << choiceToOutput(computerChoice)
          << " And " << winnerToOutput(winner)
          << "\n";

---

Are you trying to draw smiley faces in the code:

 :(" : "It's a draw :)");

Fun. But stop it.

 : " : "It's a draw :)";