Rock Paper Scissor Code

Question

Here is the code. Works great! Any advice would be great! Also, try it BEFORE reading the code. See if it is good. I've only found 1 "exploit" that can be use to beat the computer everytime and it is still in progress.

NOTE : All class are in one folder because of simplicity and other reasons

Git repo., you can download the .exe here : https://github.com/DatHA3990/rockpaperscissor.

How to play:

Choose between rock : press 1, paper : press 2, or scissor : press 3.

Then press ENTER.

Here is the code:

/*

Code written by Dat HA
Copyrights 2017/03/30

This code used to be in multiple folders, but do to compiler problems and many more,
I've decided to pack them all in to this one.

I used multiple - if - else if - else - because it is reliable and clean.

*/

#include <iostream> // I/O library
#include <string>   // string library
#include <vector>   // vector library

#define VERSION_NUM 1.21 // version number

#define ROCK 1
#define PAPER 2
#define SCISSOR 3

#define TIE 0

#define LOSS 1
#define WIN 2

#define HUMAN 1
#define CPU 2

// CALL UPDATE EACH TIME!!!!!
class CpuPlayer { // cpu player class
private:
    int m_move = 0; // cpu's move

    bool m_firstMove = 1; // true = this is the first move : false = we've past the first move
    static const int m_cpuFirstMove = PAPER; // cpu's first move will be paper

    int m_totalGames = 0; // total games

    std::vector<int> m_gameResult{ 0 }; // the game results - 0 - tie or error - 1 = human player win - 2 = cpu win
    std::vector<int> m_ennemiMove{ 0 }; // ennemi's moves are logged here
    std::vector<int> m_cpuMove{ 0 };    // cpu's moves are lagged here

    void FirstMove();

    void Loss();    // we previously loss
    void Win();     // we previously won
    void Tie();     // we previously tied

    void Double();  // the opponent played the same move 2 times
    void Spam();    // the opponent is spaming a move (because they are trying to crack the algo!!!!)
    void Reverse(); // opponent is going in reverse order, let's fix them!
    void Streak();  // opponent is on a streak

public:
    CpuPlayer();
    ~CpuPlayer();

    int GetMove(); // the AI will generate a smart move using my algorithm
    void Update(int previousOpponentMove, int gameResult); // uptade the AI with the latest info - ennemi's move and victor
};


class HumanPlayer {
public:
    HumanPlayer();
    ~HumanPlayer();

    int GetMove();
};

class Referee {
public:
    Referee();
    ~Referee();

    // return 0 = tie, 1 = human win, 2 = cpu win
    int GetScore(int humanPlayerScore, int cpuPlayerScore);
};

class Translate
{
public:
    Translate();
    ~Translate();

    std::string TieLossWin(int val);       // translate winner value
    std::string RockPaperScissor(int val); // translate move
};

CpuPlayer CpuPlayer_;     // cpu player
HumanPlayer HumanPlayer_; // human player
Referee Referee_;         // referee - the one that decides the winner
Translate Translate_;     // translate value, becaue we wan't to print it

int main() {
    std::cout << "Version : " << VERSION_NUM << std::endl << std::endl;

    while (1) {
        std::cout <<
            "Enter move : " << std::endl;

        int cpuMove = CpuPlayer_.GetMove();     // cpu player makes its move - move generated BEFORE the human player
        int humanMove = HumanPlayer_.GetMove(); // human player makes its move

        int victory = Referee_.GetScore(humanMove, cpuMove); // compare the results to get the match's victor

        std::cout << // print
            Translate_.RockPaperScissor(humanMove) << // human move
            "  vs  " << // vs
            Translate_.RockPaperScissor(cpuMove) << // cpu move
            "  :  " << // :
            Translate_.TieLossWin(victory) << // winner
            std::endl << std::endl; // skip a line

        CpuPlayer_.Update(humanMove, victory); // update the cpu player with its new info
    }
    return 0;
}

CpuPlayer::CpuPlayer() {}
CpuPlayer::~CpuPlayer() {}

// The code is written the way it is because this method is flexible

void CpuPlayer::FirstMove() {
    m_move = m_cpuFirstMove;
    m_firstMove = 0;
}

void CpuPlayer::Loss() {
    if (m_ennemiMove[m_totalGames] == ROCK)
        m_move = PAPER;
    else if (m_ennemiMove[m_totalGames] == PAPER)
        m_move = SCISSOR;
    else if (m_ennemiMove[m_totalGames] == SCISSOR)
        m_move = ROCK;
}

void CpuPlayer::Win() {
    if (m_move == ROCK)
        m_move = PAPER;
    else if (m_move == PAPER)
        m_move = SCISSOR;
    else if (m_move == SCISSOR)
        m_move = ROCK;
}

void CpuPlayer::Tie() {
    if (m_move == ROCK)
        m_move = SCISSOR;
    else if (m_move == PAPER)
        m_move = ROCK;
    else if (m_move == SCISSOR)
        m_move = PAPER;
}

void CpuPlayer::Double() {
    if (m_ennemiMove[m_totalGames] == ROCK)
        m_move = SCISSOR;
    else if (m_ennemiMove[m_totalGames] == PAPER)
        m_move = ROCK;
    else if (m_ennemiMove[m_totalGames] == SCISSOR)
        m_move = PAPER;
}

void CpuPlayer::Spam() {
    if (m_ennemiMove[m_totalGames] == ROCK)
        m_move = PAPER;
    else if (m_ennemiMove[m_totalGames] == PAPER)
        m_move = SCISSOR;
    else if (m_ennemiMove[m_totalGames] == SCISSOR)
        m_move = ROCK;
}

void CpuPlayer::Reverse() {
    if (m_ennemiMove[m_totalGames] == ROCK)
        m_move = ROCK;
    else if (m_ennemiMove[m_totalGames] == PAPER)
        m_move = PAPER;
    else if (m_ennemiMove[m_totalGames] == SCISSOR)
        m_move = SCISSOR;
}

void CpuPlayer::Streak() {
    if (m_ennemiMove[m_totalGames] == ROCK)
        m_move = ROCK;
    else if (m_ennemiMove[m_totalGames] == PAPER)
        m_move = PAPER;
    else if (m_ennemiMove[m_totalGames] == SCISSOR)
        m_move = SCISSOR;
}

int CpuPlayer::GetMove() {
    if (m_firstMove) // first move
        this->FirstMove();
    else if ((m_totalGames > 3) && (m_ennemiMove[m_totalGames] == m_ennemiMove[m_totalGames - 1]) &&
        (m_ennemiMove[m_totalGames] == m_ennemiMove[m_totalGames - 2])) // if the ennemi is spamming
        this->Spam();

    else if (m_totalGames >= 1 && (
        (m_ennemiMove[m_totalGames] == ROCK && m_ennemiMove[m_totalGames - 1] == PAPER) ||
        (m_ennemiMove[m_totalGames] == PAPER && m_ennemiMove[m_totalGames - 1] == SCISSOR) ||
        (m_ennemiMove[m_totalGames] == SCISSOR && m_ennemiMove[m_totalGames - 1] == ROCK)))
        this->Reverse();

    else if (m_totalGames >= 3 && (
        m_gameResult[m_totalGames] == LOSS &&
        m_gameResult[m_totalGames - 1] == LOSS &&
        m_gameResult[m_totalGames - 2] == LOSS))
        this->Streak();

    else if (m_gameResult[m_totalGames] == LOSS) // if we loss the previous game
        this->Loss();
    else if (m_gameResult[m_totalGames] == TIE) // if the game is a tie
        this->Tie();
    else if (m_totalGames > 2 && m_ennemiMove[m_totalGames] == m_ennemiMove[m_totalGames - 1]) // if he did 2 times the same move
        this->Double();
    else if (m_gameResult[m_totalGames] == WIN) // if we won the previous game
        this->Win();

    return m_move;
}

void CpuPlayer::Update(int previousOpponentMove, int gameResult) {
    m_totalGames++; // add a game to the total count

                    // resize the vectors for game results, ennemi's moves and cpu's moves
    m_gameResult.resize(m_totalGames + 1);
    m_ennemiMove.resize(m_totalGames + 1);
    m_cpuMove.resize(m_totalGames + 1);

    // update the information and log some info
    m_gameResult[m_totalGames] = gameResult;           // log the game result
    m_ennemiMove[m_totalGames] = previousOpponentMove; // log the ennemi's move
    m_cpuMove[m_totalGames] = m_move;                  // log the cpu's move
}

HumanPlayer::HumanPlayer() {}
HumanPlayer::~HumanPlayer() {}

int HumanPlayer::GetMove() {
    std::cout << ">>>";

    std::string string;
    std::cin >> string;

    if (string == "1")
        return ROCK;
    else if (string == "2")
        return PAPER;
    else if (string == "3")
        return SCISSOR;
    else this->GetMove();
}

Referee::Referee() {}
Referee::~Referee() {}

int Referee::GetScore(int humanPlayerScore, int cpuPlayerScore) {
    if ((humanPlayerScore == ROCK    && cpuPlayerScore == SCISSOR) ||
        (humanPlayerScore == PAPER   && cpuPlayerScore == ROCK) ||
        (humanPlayerScore == SCISSOR && cpuPlayerScore == PAPER))
        return HUMAN;

    else if ((humanPlayerScore == ROCK    && cpuPlayerScore == PAPER) ||
        (humanPlayerScore == SCISSOR && cpuPlayerScore == ROCK) ||
        (humanPlayerScore == PAPER   && cpuPlayerScore == SCISSOR))
        return CPU;

    else return TIE;
}

Translate::Translate() {}
Translate::~Translate() {}

std::string Translate::TieLossWin(int val) {
    if (val == 0)
        return "TIE";
    else if (val == 1)
        return "HUMAN WIN";
    else if (val == 2)
        return "CPU WIN";
    else
        return "SYSTEM ERROR";
}

std::string Translate::RockPaperScissor(int val) {
    if (val == 1)
        return "ROCK";
    else if (val == 2)
        return "PAPER";
    else if (val == 3)
        return "SCISSOR";
    else
        return "SYSTEM ERROR";
}

Answer 1

I built this and played it. I managed to win a few times! Good times.

Overall, the code is straightforward and easy to understand, so props on that. Here are a few things that could use improvement.

Playability

First, I think you should print out the instructions, at least the first time the user plays. Just what you have written in your description above the code would suffice. My first instinct was to type "Rock" instead of "1". Which leads to the next point:

Error handling

There's no error checking at all in this. If the user enters something incorrect, nothing happens. No error message, no exiting the program, it just sits at the prompt and waits. I'd expect it to tell me the input was invalid and to attempt to get it again. Something like this:

int HumanPlayer::GetMove() {
    int result = ROCK;
    bool valid = false;
    do {
        std::cout << ">>>";

        std::string string;
        std::cin >> string;

        if (string == "1")
        {
            result = ROCK;
            valid = true;
        }
        else if (string == "2")
        {
            result = PAPER;
            valid = true;
        }
        else if (string == "3")
        {
            result = SCISSOR;
            valid = true;
        }
        else 
        {
            std::cout << "Invalid input. Please enter 1, 2, or 3.\n";
        }
    } while (!valid);

    return result;
}

Bugs

Notice that I removed the call to this->GetMove() in the final else clause. I did this because it was causing 2 bugs:

1. When it executes, the return value of the function is undefined
2. It can cause an infinite recursion, which eventually crashes the app. In my case I hit Cntl-D (End of File) and it spit out hundreds of ">>>"s and then crashed when the stack ran into the heap.

By making it non-recursive we eliminate the possibility of overflowing the stack. We also make it easier to follow.

Use Proper Types

C++ has a decent type system. You should use it to avoid errors. Instead of having a bunch of #defines at the top of your source file, you should create some types that express your meaning. Also, you should use constants rather than macros in C++ as they are safer and include type information. I recommend something like this:

const std::string kVersionNum("1.21");

typedef enum Attack {
    kRock = 1,
    kPaper = 2,
    kScissor = 3
} Attack;

typedef enum Outcome {
    kTie = 0,
    kLoss = 1,
    kWin = 2
} Outcome;

typedef enum User {
    kHuman = 1,
    kCPU = 2
} User;

I'm using the style of prefixing constants with k. If you prefer all caps, you can use that, but I find that style a little dated and hard to read, personally. Whatever you choose, be consistent.

AI

I really like your AI! It's very clever to base your next move on either your previous moves or the human's previous moves.

Simplify

There are a few things you could simplify. Both methods of the Translate class, and the HumanPlayer::GetMove() function could be implemented with a simple lookup table. As could all the CpuPlayer moves (Loss(), Win() etc.). Here's how it would look for Translate::TieLossWin():

std::string Translate::TieLossWin(int val) {
    const std::string kResultStrings[] = {
        std::string("TIE"),
        std::string("HUMAN WIN"),
        std::string("CPU WIN")
    };
    if (val < 3)
    {
        return kResultStrings [ val ];
    }
    else
    {
        return "SYSTEM ERROR";
    }
}

For HumanPlayer::GetMove() you could translate their string into an int and then use it as a look-up in a table similar to the above example. (Or just static_cast the int to an Attack enum.)

Answer 2

In short, your game works just fine, and it is pretty easy to read to due to the generous use of whitespace and newlines.

Just wanted to point out, however, that even if your if-statement has only one line of execution, it is better to put that inside curly braces. I recieved this tip from another post on Code Review, and I found it to be a ptty valid point. This way, if you ever want to add another line to your if-statement, you can just add it without having to add curly braces around the expression.

So instead of doing:

if (m_ennemiMove[m_totalGames] == ROCK)
        m_move = ROCK;

I would recommend:

if (m_ennemiMove[m_totalGames] == ROCK)
{
    m_move = ROCK;
}

All the other points of your code are thoroughly covered by user1118321, but I just wanted to point this out as well. Good luck!

---

As an aside, Apple's source code had an infamous bug, called goto fail. It discovered from their open source code posted. The reason for this bug? A missing curly braces for an if-statement:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;

Only one goto fail line was required, hence the curly braces were unrequired. However, by accidentaly adding another duplicate line, the code now looked to the compiler like this:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
{
    goto fail;
}
goto fail;

This led to the goto fail statement to be run even if the if-statement was falsified.

The bug was:

A duplicate line of code, goto fail;, causes a critical SSL certificate verification algorithm to jump out of a series of validations at an unexpected time, causing a success value to be returned, and thus rendering the service vulnerable to attacks. - Reflections on Curly Braces – Apple’s SSL Bug and What We Should Learn From It

Thanks to @Graipher for pointing that interesting piece of information out :)

Answer 3

Regarding the code style I have some observations. This is based on the book "Clean Code" by Uncle bob and my experiences as a software engineer.

    int m_move = 0; // cpu's move

If a name needs a comment to explain it, it's a bad name!

What does m_move mean? "cpu's move" isn't a good comment. By the way, there should be as little explanatory comments as possible and "cpu's move" does not explain what is the variable meaning. Does it say if it is the CPU turn? or does it say how many moves has the CPU done? No one can tell.

    bool m_firstMove = 1; // true = this is the first move : false = we've past the first move

Shouldn't it be better just to put

    bool isThisTheFirstMove = true;

Note that we won't need a comment to explain the variable because its name does it! One of the advantages of this is that whenever you see the variable you see the complete information. Compare

    if( m_firstMove ) //do_stuff

to

    if( isThisTheFirstMove ) //do_stuff 

When the reader sees the first one, he needs to remember what "m_firstMove" meant. On the other hand, if you read "isThisTheFirstMove" you are given the whole information and you don't need to go to the declaration of the variable to see what that means.

    static const int m_cpuFirstMove = PAPER; 

This is a good name!

    std::vector<int> m_gameResults{ 0 }; 
    std::vector<int> m_enemyRealizedMoves{ 0 }; 
    std::vector<int> m_cpuRealizedMoves{ 0 };  

A collection should be plural, and names should be in English.

Here we see and example of what I said

    int CpuPlayer::GetMove() {
    if (m_firstMove) // first move
        this->FirstMove();

Compare to

    int CpuPlayer::GetMove() {
    if ( isThisTheFirstMove )
        this->FirstMove();

Not only did we eliminate the need of a comment but also we increased the readiblity. In my head it goes like "if m_firstMove" (...what was that?) "// first move" (ah if this is the first move) you are making the reader stop and think what was the variable with the weird name used for. but if we read "if isThisTheFirstMove, then play the first move". Easier for the reader an cleaner code, showing intent and maningfull name.

You should take a look a the book "Clean code" by Robert Martin. You could improve method names, Take a look at your method and variable names.

Another piece of advice that I could give you is this.

Consider,

    else if ((m_totalGames > 3) && (m_ennemiMove[m_totalGames] == m_ennemiMove[m_totalGames - 1]) &&
    (m_ennemiMove[m_totalGames] == m_ennemiMove[m_totalGames - 2])) // if the ennemi is spamming
    this->Spam();

What if you had this

    bool CpuPlayer::EnemyIsSpamming(){
        return (m_totalGames > 3) && (m_ennemiMove[m_totalGames] == m_ennemiMove[m_totalGames - 1]) &&
    (m_ennemiMove[m_totalGames] == m_ennemiMove[m_totalGames - 2])
    }        

Then the code would read like this

    else if (EnemyIsSpamming())
        this->Spam();

Which is a LOT clearer and you don't need the comment because the method speaks for itself!

The next else,

     else if (m_totalGames >= 1 && (
    (m_ennemiMove[m_totalGames] == ROCK && m_ennemiMove[m_totalGames - 1] == PAPER) ||
    (m_ennemiMove[m_totalGames] == PAPER && m_ennemiMove[m_totalGames - 1] == SCISSOR) ||
    (m_ennemiMove[m_totalGames] == SCISSOR && m_ennemiMove[m_totalGames - 1] == ROCK)))
    this->Reverse();

Maybe this?

     else if (EnemmyIsReversing())
        this->Reverse();

If you did that with every awful-to-read if statement, the method would be less than 10 lines and would be easier to read.

There is a LOT more to cover, but I would spend my entire night doing it. However, this hopefully gave you a starting point of improving your code readability, please take a look at the book and Happy Coding!