Rock, Paper & Scissors

Question

Thanks for your time, I am new to programming and I spent some days making this rock, paper & scissor game. What other possible improvements could I make after the ones I've made myself?

I've tried explaining each step as the program goes on, but I essentially at first generate a computer pick (i.e. Rock, Paper or Scissor), then ask the user for their pick (i.e. rock, paper or scissor), compare the two and depending on the game rules (Rock vs Paper results in a loss for the Rock), output the game result.

Again, this is my first try at it. Some improvements I've already added myself are: getline instead of cin, use of functions and switches, shortened the code and merged outputs in little space.

Thank you for helping a new-entry at coding.

main.cpp

#include "main.h"

int main(int argc, const char * argv[]) {
    //variables.
    string usrPick;
    string stringResult;
    char randPick = fnc.randPick(); //generates the first random pick.
    int gameCountr = 0;
    //beginning of the program.
    cout << endl << "GAME " << gameCountr << " ~ ";
    while (getline(cin, usrPick) && ++gameCountr) {
        //variables.
        char gameResult = fnc.gameResults(fnc.toChar(usrPick), randPick);
        //converts game result to readable text.
        switch (gameResult) {
            case 'e': stringResult = "even";
                break;
            case 'w': stringResult = "won";
                break;
            case 'l': stringResult = "lost";
                break;
            default: stringResult = "?";
                break;
        }
        //prints out the choices and who won the game.
        cout << "you: " << fnc.toChar(usrPick) << " / computer: " << randPick << " / " << stringResult << endl << endl << endl;
        //generates a new random choice.
        randPick = fnc.randPick();
        //prompts the user to input their choice to play again.
        cout << "GAME " << gameCountr << " ~ ";
    }
    return 0;
}

main.h

#ifndef main_h
#define main_h

#include <iostream>
#include <vector>

using namespace std;

class exercise2 {
public:
    char toChar(string word);
    char randPick();
    char gameResults(char usr_ch, char cmptr_ch);
private:
    char outp_s;
};

exercise2 fnc;

//converts to char the user input.
char exercise2::toChar(string inpt_s) {
    //reinitializes value.
    outp_s = NULL;
    if (inpt_s == "rock") {
        outp_s = 'R';
    } else if (inpt_s == "paper") {
        outp_s = 'P';
    } else if (inpt_s == "scissor") {
        outp_s = 'S';
    } else {
        outp_s = '?';
    }
    return outp_s;
}

//generates a random choice for the computer to play.
char exercise2::randPick() {
    //variables.
    vector<char> vctrOptions = {'R','P','S'};
    //chooses a random value in the given pool of values.
    return vctrOptions[rand() % vctrOptions.size()];
}

//prints out the game results.
char exercise2::gameResults(char usr_ch, char cmptr_ch) {
    //variables.
    outp_s = NULL;
    //checks user input and applies game rules.
    switch (usr_ch) {
        case 'R':
            switch (cmptr_ch) {
                case 'R': outp_s = 'e';
                    break;
                case 'P': outp_s = 'l';
                    break;
                case 'S': outp_s = 'w';
                    break;
            }
            break;
        case 'P':
            switch (cmptr_ch) {
                case 'R': outp_s = 'w';
                    break;
                case 'P': outp_s = 'e';
                    break;
                case 'S': outp_s = 'l';
                    break;
            }
            break;
        case 'S':
            switch (cmptr_ch) {
                case 'R': outp_s = 'l';
                    break;
                case 'P': outp_s = 'w';
                    break;
                case 'S': outp_s = 'e';
                    break;
            }
            break;
        default:
            outp_s = '0';
            break;
    }
    return outp_s;
}

#endif

Answer 1

• Don't use using namespace std

• You forgot to include <string> and <cstdlib>.

• exercise2 is not a very clear name.

• Why is exercise2 fnc; global?

• char exercise2::toChar(string inpt_s) can be by const ref instead of by value.

• outp_s = NULL; avoid NULL, see http://www.stroustrup.com/bs_faq2.html#null

• You use a switch later but not here, why?
  Moreover you read in a string but then "convert" it to char instead of simply checking once if it's e.g. r or rock.

• You throw away your vector of choices everytime you call the function. Consider keeping it as a member instead.

• Look at <random> if you need randomization.

• gameResults is convoluted, you should look into another way of handling the logic.

Too many chars and too many switches. Some of your comments are unnecessary.
Try improving your program and after you did that you can look at this incredibly nice implementation of RPS by user Edward

Answer 2

Here are a couple of random suggestions. See if any of these are helpful.

• Make the prompt more explicit to the user, maybe: 'enter rock or paper or scissors or quit: '

• It is considered good practice to not use 'using namespace std:=;', instead prefix all the std symbols with std::. So std::cout, std:cin, std::end, etc.. For an initial and small project 'using namespace std;' seems okay.

• Have the compiler report warnings. For clang these are some good flags: '-Wall -Wextra -Weverything'

• out_s should be declared as a char. so 'char outp_s = NULL;'

Answer 3

Well, first of all, decouple internal and external representation. This allows for efficient and elegant implementation of the game-logic, as well as easing any changes to the user-interface you might ever want to make.

enum status { TIE, WIN, LOSS, UNKNOWN};
enum symbol { ROCK, PAPER, SCISSORS, UNKNOWN};

constexpr status resolve_game(symbol user, symbol gaia) noexcept {
    return (3 + user - gaia) % 3;
}

constexpr symbol to_symbol(std::string_view s) noexcept {
    return s == "rock" ? ROCK
        : s == "paper" ? PAPER
        : s == "scissors" ? SCISSORS
        : UNKNOWN;
}

constexpr std::string_view to_string(symbol s) noexcept {
    constexpr std::string_view names[] = { "rock", "paper", "scissors", "?", };
    return names[s];
}

constexpr std::string_view to_string(status s) noexcept {
    constexpr std::string_view names[] = { "tied", "won", "lost", "?", };
    return names[s];
}

Having done that should allow you to make the rest much nicer.

---

A couple points to coding-style:

1. Use nullptr, never NULL. The latter can cause ambiguity.

2. Namespaces std is not designed to be imported wholesale, so desist.

3. Only use std::endl if you really need to flush the stream. And in that case, prefer std::flush to demonstrate your conscious choice.

4. Header-files are for declarations, some inline-implementations, the rare preprocessor-macro and certain other constants. General function- and variable-definitions belong in implementation-files.

5. The name is an integral part of any symbol, and well-selected names clarify the code and obviate the need for most comments.

6. When you have the choice, avoid heap-allocations, even if properly encapsulated.