patterncppMinor
Object-oriented rock-paper-scissors game
Viewed 0 times
paperscissorsgamerockorientedobject
Problem
This is my code for a simple rock-paper-scissors game. Users have the choice between playing against a computer or watching a "computer player" play another "computer player".
The code for the computer vs computer mode isn't finished yet, so you must chose human vs computer mode for the program to work as expected.
If you guys have any suggestions, corrections, recommendations, etc., please share them.
The code for the computer vs computer mode isn't finished yet, so you must chose human vs computer mode for the program to work as expected.
If you guys have any suggestions, corrections, recommendations, etc., please share them.
#include
#include
#include
#include
#include
class Player{
public:
void play(); //for the computer players only
std::string hand;
int hands_won;
std::string name;
};
void Player::play(){
std::string choices[3]{"rock", "paper", "scissors"};
hand = choices[std::rand()%3]; //select rock, paper, or scissors at random
}
std::vector players(2);
void print_winner(int mode);
bool GAME_OVER();
void game_start(){
std::cout > mode;
if (mode == 1){
std::cout > players[0].name;
players[1].name = "Computer";
}
if (mode == 3){
players[0].name = "Computer 1";
players[1].name = "Computer 2";
}
while (mode == 1){ //game loop for mode 1
while (!GAME_OVER()){
std::cout > players[0].hand;//player makes choice
players[1].play();//computer makes its choice
std::cout > players[0].hand;
players[1].play();
std::cout > choice;
switch (choice){
case 1:
game_start();
break;
case 2:
return 0;
default:
std::cout ";
char input;
std::cin >> input;
if (input == 'y'){
game_start();
}
return 0;
}
}
bool GAME_OVER(){
if(players[0].hands_won == 2 || players[1].hands_won == 2){ //best 2/3
return true;
}
return false;
}Solution
Things that definitely need fixing
-
Line 52 has:
That is compiler error. It needs to be:
-
You have lines 60 and 63:
These lines are in a function that has
Inconsistent naming convention
All of your functions are consistently named except
Inconsistent physical layout
All of your functions are declared and defined before
Personally, I prefer the declarations of all the functions to be at the top of the file irrespective of whether they are defined before
Names of functions
This maybe a personal preference, but I think function names that start with a verb seem better than those that start with a noun.
Use
Use
Using
Instead of using magic numbers like
Make functions still smaller
You have functions that accept input, process the input, and display the result of processing the input. You can separate the first part the last part into separate functions. That will make your code easier to follow and maintain. For example,
Make
You can simplify the code in a few places by making
Refactored Program
-
Line 52 has:
std::cout << endl;That is compiler error. It needs to be:
std::cout << std::endl;-
You have lines 60 and 63:
return 1;
return 0;These lines are in a function that has
void as return type. These also produce compile errors. They need to be changed to:return;
return;Inconsistent naming convention
All of your functions are consistently named except
GAME_OVER. That should be changed to game_over to be consistent.Inconsistent physical layout
All of your functions are declared and defined before
main except GAME_OVER which is defined after main. I would recommend putting the definitions of all the helper function before main or all of them after main. Having some of them before main and some of them after main is inconsistent.Personally, I prefer the declarations of all the functions to be at the top of the file irrespective of whether they are defined before
main or after main.Names of functions
This maybe a personal preference, but I think function names that start with a verb seem better than those that start with a noun.
Use
start_game instead of game_startUse
is_game_over instead of game_overprint_winner already starts with a verb.Using
enums for user choicesInstead of using magic numbers like
1 and 2, I would recommend using enums. That will make the code more readable.enum TopLevelChoice { PLAY_GAME, EXIT_GAME };
enum PlayMode { HUMAN_VS_COMPUTER, COMPUTER_VS_COMPUTER };Make functions still smaller
You have functions that accept input, process the input, and display the result of processing the input. You can separate the first part the last part into separate functions. That will make your code easier to follow and maintain. For example,
main can be:int main()
{
std::srand(time(NULL));
while ( get_toplevel_choice() != EXIT_GAME )
{
play_game();
}
return 0;
}Make
Player a base classYou can simplify the code in a few places by making
Player a base class and creating two sub-classes: HumanPlayer and ComputerPlayer.Refactored Program
#include
#include
#include
#include
#include
class Player
{
public:
Player(std::string const& name) : name(name), hands_won(0) {}
virtual ~Player() {}
virtual void play() = 0;
virtual void printWinningMessage() = 0;
std::string hand;
std::string name;
int hands_won;
};
class HumanPlayer : public Player
{
public:
HumanPlayer(std::string const& name) : Player(name) {}
virtual void play()
{
std::cout > hand;//player makes choice
}
virtual void printWinningMessage()
{
std::cout > choice;
if ( std::cin ) // Reading was successful.
{
switch (choice)
{
case 1:
return PLAY_GAME;
case 2:
return EXIT_GAME;
default:
std::cout ::max(), '\n');
// Try again.
return get_toplevel_choice();
}
}
PlayMode get_play_mode()
{
std::cout > mode;
if ( std::cin ) // Reading was successful.
{
switch (mode)
{
case 1:
return HUMAN_VS_COMPUTER;
case 2:
return COMPUTER_VS_COMPUTER;
default:
std::cout ::max(), '\n');
// Try again.
return get_play_mode();
}
}
void play_game()
{
PlayMode choice = get_play_mode();
if ( choice == HUMAN_VS_COMPUTER )
{
play_human_vs_computer();
}
else
{
play_computer_vs_computer();
}
}
void play_game(Player& player1, Player& player2)
{
do
{
player1.play();
player2.play();
//display each player's choice for the round
std::cout > name;
HumanPlayer humanPlayer(name);
ComputerPlayer computerPlayer("Computer");
play_game(humanPlayer, computerPlayer);
}
void play_computer_vs_computer()
{
ComputerPlayer computerPlayer1("Computer 1");
ComputerPlayer computerPlayer2("Computer 2");
play_game(computerPlayer1, computerPlayer2);
}
void pick_round_winner(Player& player1, Player& player2)
{
// We have already taken care of the draw.
if ( (player1.hand == "rock" && player2.hand == "scissors") ||
(player1.hand == "scissors" && player2.hand == "paper") ||
(player1.hand == "paper" && player2.hand == "rock") )
{
player1.printWinningMessage();
player1.hands_won += 1;
}
else
{
player2.printWinningMessage();
player2.hands_won += 1;
}
}
void print_winner(Player& player1, Player& player2)
{
Player& winner = (player1.hands_won == 2) ? player1 : player2;
std::cout << "Congratulations, " << winner.name << ". You win the game!\n";
}
bool is_game_over(Player& player1, Player& player2)
{
return (player1.hands_won == 2 || player2.hands_won == 2);
}Code Snippets
std::cout << endl;std::cout << std::endl;return 1;
return 0;return;
return;enum TopLevelChoice { PLAY_GAME, EXIT_GAME };
enum PlayMode { HUMAN_VS_COMPUTER, COMPUTER_VS_COMPUTER };Context
StackExchange Code Review Q#90588, answer score: 5
Revisions (0)
No revisions yet.