Simple rock-paper-scissors game [Follow Up 1]

Question

This is a follow my rock-paper-scissors program, original post here: Simple rock-paper-scissors game

Looking for any feedback and recommendations on ways to keep improving.

Quick notes on changes:

• Removed the use of using namespace std;
• Added inheritance structure.
• Added the use of enum variables to replace strings to ease computational expense while still maintaining code readability.
• Simplified the logic of the whoWon function.
• Changed the scope whoWon function to deal with a singular task
• Removed the use of rand and replaced it with a normal distribution from the random library.
• Added the option of choosing from three types of play: Player vs Computer, Player vs Player(Local), and Computer vs Computer.
• Move choices given by Human players will no longer be echoed onto the application window to ensure player's do not unfairly see each-other's choices.
• Added a moveToString function so that each player's move choice can be displayed after the winner is determined.
• Changed main.cpp loop structure to be do-while, so it will run atleast once.

Code:

rock-paper-scissors_v2.h:

#ifndef ROCK_PAPER_SCISSORS_V2_H
#define ROCK_PAPER_SCISSORS_V2_H

#include <iostream>
#include <random>
#include <string>
#include <unordered_map>
#include <termios.h>
#include <unistd.h>
enum  rock_paper_scissors{
        rock = 0,
        paper = 1,
        scissors = 2
    };

class Player{

protected:

    rock_paper_scissors move_choice;

public: 
   virtual void setMoveChoice() = 0;
   virtual rock_paper_scissors getMoveChoice() = 0;
   std::string moveToString(rock_paper_scissors);
};


class ComputerPlayer : public Player{

public:
    void setMoveChoice();
    rock_paper_scissors getMoveChoice();

};

class HumanPlayer : public Player{

private:
    std::string player_name;

public:
    HumanPlayer();
    void setMoveChoice();
    rock_paper_scissors getMoveChoice();

};

#endif /* ROCK_PAPER_SCISSORSS_H */

rock-paper-scissors_v2.cpp:

#include "rock-paper-scissors_v2.h"

using std::cin;
using std::cout;

std::string Player:: moveToString(rock_paper_scissors move){
    std::string move_as_string;
    if(move == rock)
        move_as_string = "rock";
    else if(move == paper)
        move_as_string = "paper";
    else
        move_as_string = "scissors";

    return move_as_string;
}
void ComputerPlayer :: setMoveChoice(){

   //Simulate the computer randomly choosing a move 
    std::random_device rd;  
    std::mt19937 gen(rd());
    std::uniform_int_distribution<int> distribution(0,2);
    int num_generated = distribution(gen);

    move_choice= (rock_paper_scissors)num_generated;                        
}

rock_paper_scissors ComputerPlayer :: getMoveChoice(){
    return move_choice;
}


HumanPlayer :: HumanPlayer(){
    cout<<"Please enter your name: ";
    cin>>player_name;
    cout<<"Hello "<<player_name<<", welcome to Rock-Paper-Scissors! \n";

}

void HumanPlayer :: setMoveChoice(){
    cout<<player_name<<", please enter the move you wish to make: \n";

    termios oldt;
    tcgetattr(STDIN_FILENO, &oldt);
    termios newt = oldt;
    newt.c_lflag &= ~ECHO;
    tcsetattr(STDIN_FILENO, TCSANOW, &newt);

    std::string move;
    cin>>move;

    //Use a hashmap to map user string input to enum
    std::unordered_map<std::string,rock_paper_scissors> valid_moves = {
        {"rock",rock},{"paper",paper},{"scissors",scissors}
    };

    //Check for valid user input
    while(valid_moves.find(move) == valid_moves.end()){
        cout<<"Error, invalid move! Please enter rock, paper, or scissors \n";
        getline(cin,move);
    }

    move_choice = valid_moves[move];

}

rock_paper_scissors HumanPlayer :: getMoveChoice(){
    return move_choice;
}

main.cpp:

#include "rock-paper-scissors_v2.h"
using std::cin;
using std::cout;

int whoWon(rock_paper_scissors &p1_move, rock_paper_scissors &p2_move){

// Function will return 1 if player 1 wins, 2 if player 2 wins, and 3 for a tie

//Both players making the same move results in a tie
    if(p1_move == p2_move){
        return 3;
    }
 //If not a tie, check for player 1 being the winner   
    else if((p1_move == rock && p2_move == scissors) || (p1_move == paper && p2_move == rock) || (p1_move == scissors && p2_move == paper)){
        return 1;
    }

 // If none of the above are true, player 2 wins
    else{
        return 2;
    }
}

int main(int argc, char** argv) {
 char play_again;
 do{

 //Initiate the Title Screen
     cout<<"Rock-Paper-Scissors \n";
     cout<<"-------------------\n";
     cout<<"Enter the number of the mode you wish to play: \n";
     cout<<"1.) Player vs Computer \n2.) Player vs Player(Local Only) \n3.) Computer vs Computer(Simulation) \n";
     int mode_selected;
     cin>>mode_selected;

     Player *player_1;
     Player *player_2;


     switch(mode_selected){
         case 1:{
            ComputerPlayer com;
            HumanPlayer hum;

            player_1 = &hum;
            player_2 = &com;

            break;
         }

         case 2:{
             HumanPlayer hum_1;
             HumanPlayer hum_2;


             player_1 = &hum_1;
             player_2 = &hum_2;

             break;

         }

         case 3:{
          ComputerPlayer com_1;
          ComputerPlayer com_2;

          player_1 = &com_1;
          player_2 = &com_2;

          break;
         }
     }

    player_1->setMoveChoice();
    rock_paper_scissors p1_move = player_1->getMoveChoice();

    player_2->setMoveChoice();
    rock_paper_scissors p2_move = player_2->getMoveChoice();  

    int winner = whoWon(p1_move,p2_move);


    if(winner == 1){
             cout<<"Player one chose "<<player_1->moveToString(p1_move)<<" and Player 2 chose "<<player_1->moveToString(p2_move)<< ". Player 1 wins! \n";

    }

    else if(winner == 2){
          cout<<"Player one chose "<<player_1->moveToString(p1_move)<<" and Player 2 chose "<<player_1->moveToString(p2_move)<< ". Player 2 wins! \n";

    }

     else{
        cout<<"Player one chose "<<player_1->moveToString(p1_move)<<" and Player 2 chose "<<player_1->moveToString(p2_move)<< ". It's a tie! \n";

    }

   cout<<"Would you like to play again?(y/n) \n";
   cin>>play_again;



 } while(play_again == 'y');




 return 0;
}

Additional Questions:

• What are some good sources to look at for adding simple PvP online play?
• Any tips/ resource recommendations for improving the current "menu" UI?

Answer 1

Visually, your indentation and spacing are a little inconsistent, making parts of the code harder to read.

You should include the override keyword in your ComputerPlayer and HumanPlayer classes if your compiler supports it.

The getMoveChoice functions can be const since they are not changing the class's value.

You can use a switch statement in moveToString instead of multiple if statements. Or, since the enum values are consecutive, look up the strings in a static array. A third alternative would be to use a data structure you can share with setMoveChoice so you don't need to duplicate the strings and values. moveToString can be a static member of Player as it doesn't reference any of the non-static members of the class.

In whoWon, you can create a (static) data structure to hold what values for player1 and player2 will be a win for player1 (which you'd check after seeing if it is a tie). Or use a switch statement rather than the multiple conditions in the if.

The problems in main are more serious. In the switch statement, you declare local variables, assign a pointer to them, the those locals go out of scope. This will call the destructors for your players and result in Undefined Behavior when you later access them thru the pointers. You also do not validate your input, which can result in uninitialized pointers for your players.

When displaying the results, you have a lot of repetition when you state the initial choices. This can be output before determining how to display the results (which, again, could be a switch instead of cascading ifs).

Lastly, all your string and character comparisons are case sensitive, so the user has to enter everything in lowercase. (This isn't necessarily a problem, though.)

The random device and generator in ComputerPlayer :: setMoveChoice should be members of the ComputerPlayer class, and not local variables. You only want to create them once. Continually making new instances of them can lead to non-random results.

In HumanPlayer :: setMoveChoice the valid_moves data could be combined with the data in moveToString, as previously mentioned. If you keep it where it is, it should be static (so it is only created once). And while an unordered_map works, my personal preference would be to use a simpler data structure (search thru a C style array) since there are only three elements. You're also inconsistent in how you get the move from the player. The initial input is with cin>>move, while the reentry on an invalid input is getline. You should use the same method (probably getline).