3
\$\begingroup\$

How can I optimize this code? I want to use more C++11 features.

game_control.h

#ifndef GAME_CONTROL_H
#define GAME_CONTROL_H

#include <iostream>
#include <array>
#include <string>

class Game
{
    unsigned int number_of_turns = 0;
    unsigned int user_score = 0, comp_score = 0;
    std::array<std::string, 3> data = {{"ROCK", "PAPER", "SCISSOR"}};
    std::string user_name;

  public:
    Game() {};
    ~Game() = default;

    void start_menu();
    void start_game();
    void game_play(unsigned int, unsigned int);
    void exit_game();
};

#endif

game_control.cpp

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>

#include "game_control.h"


void Game::start_menu()
{
    std::cout << "####  ####  ####  #  #   ####  ####  ####  ####  ####\n";
    std::cout << "#  #  #  #  #     # #    #  #  #  #  #  #  #     #  #\n";
    std::cout << "####  #  #  #     ##     ####  ####  ####  ####  ####\n";
    std::cout << "# #   #  #  #     # #    #     #  #  #     #     # # \n";
    std::cout << "#  #  ####  ####  #  #   #     #  #  #     ####  #  #\n";
    std::cout << "\n\n";
    std::cout << "   ####  ####  #  ####  ####  ####  ####\n";
    std::cout << "   #     #     #  #     #     #  #  #  #\n";
    std::cout << "   ####  #     #  ####  ####  #  #  ####\n";
    std::cout << "      #  #     #     #     #  #  #  # # \n";
    std::cout << "   ####  ####  #  ####  ####  ####  #  #\n";
    std::cout << "\n\n";

    unsigned int selection;
    x:
    std::cout << "\tPress\n";
    std::cout << "\t1 to Enter\n";
    std::cout << "\t2 to Exit\n";
    std::cin >> selection;
    std::cin.clear();

    switch(selection)
    {
        case 1: start_game();
                break;
        case 2: exit_game();
                break;
        default: std::cout << "-------Enter valid choice--------\n";
                 goto x;
                 break;
    }
}

void Game::exit_game()
{
    std::cout << "Thanks for playing!!\n";
    exit(EXIT_SUCCESS);
}

void Game::start_game()
{
    std::cout << "\nEnter your name\n";
    //std::getline(std::cin, user_name);
    std::cin >> user_name;
    std::cin.clear();

    std::cout << "Enter number of turns for you want to play\n";
    std::cin >> number_of_turns;
    std::cin.clear();

    while (number_of_turns--)
    {
        unsigned int user_choice, comp_choice;
        std::cout << "\nEnter your choice\n";
        y:
        std::cout << "\nPress\n";
        std::cout << "1 for ROCK\t2 for PAPER\t3 for SCISSOR\n";
        std::cin >> user_choice;
        if (user_choice > 3)
        {
            std::cout << "\n---------Please enter valid choice------------\n";
            goto y;
        }
        user_choice--;
        std::cin.clear();

        srand(time(0));
        comp_choice = rand() % 3 + 1;

        game_play(user_choice, comp_choice);
        std::cout << "The score is\t";
        std::cout << user_name << " = " << user_score;
        std::cout << "\tComputer = " << comp_score << "\n";
    }
    exit_game();
}

void Game::game_play(unsigned int user_choice, unsigned int comp_choice)
{
    //User = Rock
    if (user_choice == 0)
    {
        if (comp_choice == 1) // Computer = Paper
        {
            std::cout << "\n";
            std::cout << user_name << " chooses " << data[user_choice];
            std::cout << " and Computer chooses " << data[comp_choice] << "\n";
            std::cout << "----Computer Wins!!----\n";
            comp_score++;
        }
        else if (comp_choice == 2) // Computer = Scissor
        {
            std::cout << "\n";
            std::cout << user_name << " chooses " << data[user_choice];
            std::cout << " and Computer chooses " << data[comp_choice] << "\n";
            std::cout << "----" << user_name << " Wins!!----\n";
            user_score++;
        }
        else
        {
            std::cout << "\n";
            std::cout << "Both " << user_name << " and Computer chooses ROCK\n";
        }
    }

    //User = Paper
    if (user_choice == 1)
    {
        if (comp_choice == 0) // Computer = Rock
        {
            std::cout << "\n";
            std::cout << user_name << " chooses " << data[user_choice];
            std::cout << " and Computer chooses " << data[comp_choice] << "\n";
            std::cout << "----" << user_name << " Wins!!----\n";
            user_score++;
        }
        else if (comp_choice == 2) // Computer = Scissor
        {
            std::cout << "\n";
            std::cout << user_name << " chooses " << data[user_choice];
            std::cout << " and Computer chooses " << data[comp_choice] << "\n";
            std::cout << "-----Computer Wins!!-----\n";
            comp_score++;
        }
        else
        {
            std::cout << "\n";
            std::cout << "Both " << user_name << " and Computer chooses PAPER\n";
        }
    }

    //User = Scissor
    if (user_choice == 2)
    {
        if (comp_choice == 0) // Computer = Rock
        {
            std::cout << "\n";
            std::cout << user_name << " chooses " << data[user_choice];
            std::cout << " and Computer chooses " << data[comp_choice] << "\n";
            std::cout << "----Computer Wins!!-----\n";
            comp_score++;
        }
        else if (comp_choice == 1) // Computer = Paper
        {
            std::cout << "\n";
            std::cout << user_name << " chooses " << data[user_choice];
            std::cout << " and Computer chooses " << data[comp_choice] << "\n";
            std::cout << "----" << user_name << " Wins!!----\n";
            user_score++;
        }
        else
        {
            std::cout << "\n";
            std::cout << "Both " << user_name << " and Computer chooses SCISSOR\n";
        }
    }
}

main.cpp

#include <iostream>
#include "game_control.h"

int main()
{
    Game game;
    game.start_menu();
    game.start_game();

    return 0;
}
|improve this question
\$\endgroup\$
  • 3
    \$\begingroup\$ Does it work as intended? \$\endgroup\$ – Mast Jun 16 '18 at 16:56
10
\$\begingroup\$

I'll go through your code and give comments on some chunks of it. Horizontal lines separate the discussion of different chunks.

    Game() {};
    ~Game() = default;

  1. You define a public constructor which doesn't do anything. Why define it at all? If it's intended to be trivial, and you really want to include it, it's better to explicitly say Game()=default;.

  2. If you go with the explicitly defined constructor, the trailing semicolon is extraneous.

  3. The destructor is similar to point 1: there's no need to explicitly say that it's default, unless you want to modify it like e.g. adding virtual keyword to it.

    std::array<std::string, 3> data = {{"ROCK", "PAPER", "SCISSOR"}};

The name of this variable is not quite useful. I'd rather name it something like choice_names.

    std::cin >> selection;
    std::cin.clear();

    switch(selection)

You do clear error flags when the user enters something not parseable as an unsigned int, but you forget to clear the read buffer. Thus, on entry of something other than a valid unsigned int (e.g. a letter), your program will hang in infinite loop of requesting a valid choice and re-attempting to read the invalid input. An easy fix could be something like this:

if(!std::cin)
{
    std::cin.clear();
    std::cin.ignore();
}

    x:
...
                 goto x;

You use goto to make a loop. This is already not a good idea, because the language does supply a set of constructs for looping: for, while, do...while. You should use one of these by default, and only resort to goto when no sane use of loops will solve your problem.

Moreover, even if you do use goto here, the label is badly named. Instead of x I'd name it something like show_menu.

    //std::getline(std::cin, user_name);
    std::cin >> user_name;
    std::cin.clear();

Actually using std::getline was a good idea. And current uncommented code fails to work correctly when the user enters e.g. Name Surname: the next extraction std::cin >> number_of_turns; fails, and you get number_of_turns==0. This is because extraction operator stops on white space. Using std::getline would avoid the problem.

I won't comment on further occurrences of std::cin >> ... + std::cin.clear();.

    if (user_choice == 0)
    {
        if (comp_choice == 1) // Computer = Paper
        {

These if-else if chains with the same variable compared to different constants are better transformed into switch constructs. This way you'll

  1. Avoid repetition of the variable name,
  2. Make it easier to understand that a single variable is being tested for possible variants of values, not just many unrelated conditions are chained.
    if (user_choice == 0)
    ...
    if (user_choice == 1)
    ....

Instead of hard-coding numeric values, it's better to name them. E.g.

    enum : unsigned
    {
        CHOICE_ROCK=1,
        CHOICE_PAPER,
        CHOICE_SCISSOR,

        NUM_CHOICES
    };

You can then easily print the menu like

    for(unsigned c=CHOICE_ROCK;c<NUM_CHOICES;++c)
        std::cout << c << " for " << choice_names[c-1] << (c+1==NUM_CHOICES ? '\n' : '\t');

with the benefit that you can now reorder choices (in the enumeration and in choice_names), and the other code won't require changes (when you also fix "Both " << user_name << " and Computer chooses SCISSOR\n" so that it doesn't hard-code the SCISSOR string). This is known as the DRY principle.

void Game::exit_game()
{
    std::cout << "Thanks for playing!!\n";
    exit(EXIT_SUCCESS);
}

In C++ it's better to avoid using std::exit (yes, it's in namespace std; its presence in global namespace is not guaranteed for <cstdlib>, unlike <stdlib.h>). The reason is that it doesn't call any destructors of automatic variables. In your case it doesn't matter much, since class Game doesn't need any real destruction, but generally you might lose some resources this way, since classes may use the RAII idiom to e.g. grab and release external resources, and need to e.g. close USB devices, delete temporary files, say goodbye to a remote host etc..

main.cpp: the <iostream> include is not needed there.

int main()
{
    ...
    return 0;
}

Nitpick: reaching the end of main function automatically returns 0, so this return statement is unnecessary. (Note that this only applies to main, not any other function.)

I won't repeat what the other answers say, please do read them: they make quite good points.

|improve this answer
\$\endgroup\$
  • \$\begingroup\$ If I use std::getline(std::cin, user_name) it does not take any input and directly asks for number_of_turns \$\endgroup\$ – coder Jun 18 '18 at 9:49
  • \$\begingroup\$ @coder right. This is because the input operation before it, via operator>>, got the number (1 to Enter) and left the '\n' and any other characters after the number in the input buffer. You should call std::cin.ignore() before doing std::getline on std::cin to avoid this. Test case: instead of 1, try entering 1 hello in the first menu: you'll get ` hello\n` as the user name with current code + getline. Better yet way is to validate user input completely, by using getline everywhere and checking that there's no trailing garbage in the input. \$\endgroup\$ – Ruslan Jun 18 '18 at 10:19
8
\$\begingroup\$

As you are asking for using more c++11 features specifically.

1. Use raw string literals

The way you are printing your banner looks clunky, and is hard to maintain:

std::cout << "####  ####  ####  #  #   ####  ####  ####  ####  ####\n";
std::cout << "#  #  #  #  #     # #    #  #  #  #  #  #  #     #  #\n";
std::cout << "####  #  #  #     ##     ####  ####  ####  ####  ####\n";
std::cout << "# #   #  #  #     # #    #     #  #  #     #     # # \n";
std::cout << "#  #  ####  ####  #  #   #     #  #  #     ####  #  #\n";
std::cout << "\n\n";
std::cout << "   ####  ####  #  ####  ####  ####  ####\n";
std::cout << "   #     #     #  #     #     #  #  #  #\n";
std::cout << "   ####  #     #  ####  ####  #  #  ####\n";
std::cout << "      #  #     #     #     #  #  #  # # \n";
std::cout << "   ####  ####  #  ####  ####  ####  #  #\n";
std::cout << "\n\n";

IMO this can be done way better using a raw string literal like

std::string banner = R"x(
####  ####  ####  #  #   ####  ####  ####  ####  ####
#  #  #  #  #     # #    #  #  #  #  #  #  #     #  #
####  #  #  #     ##     ####  ####  ####  ####  ####
# #   #  #  #     # #    #     #  #  #     #     # # 
#  #  ####  ####  #  #   #     #  #  #     ####  #  #


   ####  ####  #  ####  ####  ####  ####
   #     #     #  #     #     #  #  #  #
   ####  #     #  ####  ####  #  #  ####
      #  #     #     #     #  #  #  # # 
   ####  ####  #  ####  ####  ####  #  #

)x";

std::cout << banner;

2. Prefer Pseudo Number Generation over rand()

The functions and classes from the Pseudo Number Generation library, give you better control about the way the random numbers are generated:

  • ability to use hardware devices for randomizing (e.g. based on cosmic radiation detection)
  • ability to control the mean distribution of random numbers chosen
|improve this answer
\$\endgroup\$
6
\$\begingroup\$

Your game play function is excessively verbose. You execute the same lines of code in 6 different places to print out the user & computer choices:

        std::cout << user_name << " chooses " << data[user_choice];
        std::cout << " and Computer chooses " << data[comp_choice] << "\n";

You could refactor this common code to one location:

    if ( user_choice != comp_choice ) {
        std::cout << user_name << " chooses " << data[user_choice];
        std::cout << " and Computer chooses " << data[comp_choice] << "\n";
        ...
    } else {
        ...
    }

You’ve got 3 different places where you print out the “Both choose” text. You can refactor this into the else clause, by using the data[] array to print out what the selection was.

        std::cout << "Both " << user_name << " and Computer chooses " << data[user_choice] << "\n";

Finally, you’ve got 3 copies of user wins and 3 copies of player wins. This again can be reduced to one copy each, with a suitable if else statement. The player wins when they play a code 1 higher (mod 3) than the computer. Place this code in the non-tie part of the above if statement:

        if ( (user_choice % 3) == ((comp_choice+1) % 3)) {
            std::cout << "----" << user_name << " Wins!!----\n";
            user_score++;
        } else {
            std::cout << "----Computer Wins!!----\n";
            comp_score++;
        }
|improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.