5
\$\begingroup\$

I wrote this program for an exercise in Bjarne Stroustrup's text Programming -- Principles and Practice Using C++. He recommended using a vector for pseudo randomness, however I opted for srand() and rand() from cstdlib.

Any recommendations regarding efficiency or simplification would be greatly appreciated. I apologize in advance for the nested switch statements. After much thought, I could not come up with a better solution. The exercise is as follows:

Write a program that plays the game "Rock, Paper, Scissors." If you are not familiar with the game do some research (e.g., on the web using Google). Research is a common task for programmers. Use a switch statement to solve this exercise. Also, the machine should give random answers (i.e., select the next rock, paper, or scissors randomly). Real randomness is too hard to provide just now, so just build a vector with a sequence of values to be used as "the next value." If you build the vector into the program, it will always play the same game, so maybe you should let the user enter some values. Try variations to make it less easy for the user to guess which move the machine will make next.

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

void show_winner(char user_move, char comp_move)
{
    using namespace std;

    switch (user_move) {
        case 'r':
            switch (comp_move) {
                case 'r':
                    cout << "User: Rock" << endl;
                    cout << "Computer: Rock" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "Tie!" << endl;
                    break;
                case 'p':
                    cout << "User: Rock" << endl;
                    cout << "Computer: Paper" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "Computer Wins! Paper Beats Rock!" << endl;;
                    break;
                case 's':
                    cout << "User: Rock" << endl;
                    cout << "Computer: Scissor" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "User Wins! Rock Beats Scissor!";
                    break;
            }
            break;
        case 'p':
            switch (comp_move) {
                case 'r':
                    cout << "User: Paper" << endl;
                    cout << "Computer: Rock" << endl;   
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "User Wins! Paper Beats Rock!" << endl;
                    break;
                case 'p':
                    cout << "User: Paper" << endl;  
                    cout << "Computer: Paper" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "Tie!" << endl;
                    break;
                case 's':
                    cout << "User: Paper" << endl;
                    cout << "Computer: Scissor" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "Computer Wins! Scissor Beats Paper!" << endl;
                    break;
            }
            break;
        case 's':
            switch (comp_move) {
                case 'r':
                    cout << "User: Scissor" << endl;
                    cout << "Computer: Rock" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "Computer Wins! Rock Beats Scissor";
                    break;
                case 'p':
                    cout << "User: Scissor" << endl;
                    cout << "Computer: Paper" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "User Wins! Scissor Beats Paper!" << endl;
                    break;
                case 's':
                    cout << "User: Scissor" << endl;
                    cout << "Computer: Scissor" << endl;
                    cout << endl;
                    for (time_t t = time(0) + 1; time(0) < t;) {}   
                    cout << "Tie!" << endl; 
                    break;
            }
            break;
    }
}   

void countdown()
{
    using namespace std;

    // Print countdown, and wait one second after
    // each statement prints.
    cout << "Rock..." << endl;
    for (time_t t = time(0) + 1; time(0) < t;) {}   
    cout << "Paper..." << endl;
    for (time_t t = time(0) + 1; time(0) < t;) {}   
    cout << "Scissor..." << endl;
    for (time_t t = time(0) + 1; time(0) < t;) {}
    cout << endl;
    cout << "SHOOT!" << endl;
    for (time_t t = time(0) + 1; time(0) < t;) {}
}

char get_comp()
{
    // Seed the RNG
    srand(time(NULL));

    // Divide the random int by 3 and assign its 
    // remainder to a variable.
    int c_num = rand() % 3;

    // Assign a move to the computer's random number.   
    char c_move;
    switch (c_num) {
        case 0:
            c_move = 'r';
            break;
        case 1:
            c_move = 'p';
            break;
        case 2:
            c_move = 's';
            break;
    }

    // Return the Computer's Move
    return c_move;
}

char get_user()
{
    using namespace std;

    // Get the user's move
    char move;
    cout << "Enter your move (Rock = r , Paper = p , Scissor = s):" << endl;
    cin >> move;
    cout << endl;

    // Return the chosen move to the main function
    return move;
}

int main()
{
    using namespace std;

    while (true) {
        // Get the user's move
        char user_move = get_user();

        // Get the computer's move
        char comp_move = get_comp();

        // Print the countdown dialogue     
        countdown();    

        cout << endl;

        // Declare a winner 
        show_winner(user_move, comp_move);  

        for (time_t t = time(0) + 1; time(0) < t;) {}   

        /* Prompt the user to play another round.
         * Any input besides "yes","Yes","y", or "Y"
         * will break out of the main loop. */  
        string again;
        cout << endl;
        cout << "Enter Y to Play Again" << endl;
        cin >> again;   
        if (again == "Y") {}
        else break;

        cout << endl;
    }           


    // Main Function Return
    return 0;       
}
\$\endgroup\$
6
\$\begingroup\$

Rather than have big switch statements. You use a matrix of valid results:

   User Move ->       Rock      Paper      Scissor
   Comp Move  \/

            Rock      Tie       User-Win   Comp-Win
            Paper     Comp-Win  Tie        User-Win
            Scissor   User-Win  Comp-Win   Tie

This implies we should use enum

   enum Weapon  {Rock, Paper, Scissor};
   enum Result  {Tie,  Comp_Win, User_Win};

To make printing it easier add the appropriate output operators:

   std::ostream& operator<<(std::ostream& s, Weapon w)
   {
       return s << ((w == Rock) ? "Rock" : (w == Paper) ? "Paper" : "Scissor");
   }
   std::ostream& operator<<(std::ostream& s, Result r)
   {
       return s << ((r == Tie) ? "Tie" : (r == Comp_Win) ? "Comp_Win" : "User_Win");
   }

Now we can define the result matrix like this:

   Result actionMatric[3][3] = [[Tie,      User_Win, Comp_Win],
                                [Comp_Win, Tie,      User_win],
                                [User_Win, Comp_Win, Tie]];

   void show_winner(Weapon user, Weapon comp)
   {
      // Notice don't use std::endl unless you want to flush the output.
      // If you use std::endl all the time you waste the buffer that is being used
      // to make the stream effecient.
      std::cout << "User:     " << user << "\n"
                << "Computer: " << comp << "\n"
                << std::endl;
       for (time_t t = time(0) + 1; time(0) < t;) {}   
       std::cout << actionMatric[user][comp] << std::endl;
   }

Don't use a busy wait() to sleep.

for (time_t t = time(0) + 1; time(0) < t;) {}

// Prefer:  one of these.
//          Usually for multi-platform code you macro out these
sleep(1);     // Unix
Sleep(1000);  // Win  

Only seed the random number generator once in a game:

srand(time(NULL));

Easiest to right after main() is entered.

You don't need a switch to convert a random number to a value. You can calculate it (or with enum cast it).

Weapon compWeapon = static_cast<Weapon>(rand() % 3);

Don't do this:

using namespace std;
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ I don't know until now that paper is weapon. \$\endgroup\$ – user16950 Jul 7 '13 at 15:04
  • 4
    \$\begingroup\$ @AlvinWong: Never heard of a paper cut? \$\endgroup\$ – Martin York Jul 7 '13 at 22:05
3
\$\begingroup\$

As a general comments, your comments are not really helpful as they explain the how instead of the why. If you were to split your code into smaller functions and to use relevant types, most of them would not be required. You could also use the const keywords to tell the reviewer and/or the compiler that variables are not supposed to be updated.

As suggested by Jamal, an enum could make things clearer : let's define : enum Moves {ROCK, PAPER, SCISSORS};

Then you could reuse this type everywhere :

Move get_random_move()
{
    return rand() % 3;
}

Move get_user_move()
{
    for (;;)
    {
        cout << "Enter your move (Rock = r , Paper = p , Scissor = s):" << endl;
        char move;
        cin >> move;
        cout << endl;
        switch (move)
        {
            case 's': return SCISSORS;
            case 'p': return PAPER;
            case 'r': return ROCK;
            default : cout << "Invalid input, please try again." << endl;
    }
}

It would probably be worth extracting for (time_t t = time(0) + 1; time(0) < t;) {} into a function.

You should add a cout << endl; at the end of the countdown() function in order not to have to do it after you've called it.

Your show_winner functions does way too much : you should have a function handling the printing and one handling the computation of the winning player.

void get_winner(Move m1, Move m2)
{
    # Implementation to update if we update the order in the enum
    # Here we get the winner by computing the "distance" between the moves if we were to cycle on the enum in a given order.
    return (3+m1-m2)%3; # 0 is tie, 1 is move 1, 2 is move 2
}

ostream& operator<<(ostream& out, const Move &m)
{
    switch (m)
    {
        case SCISSORS: out << "Scissors"; break;
        case ROCK:     out << "Rock"; break;
        case PAPER:    out << "Paper"; break;
        default:       out << "Invalid move";
    }

    return out;
}

Then your main would be like :

int main()
{
    srand(time(NULL));
    while (true) {
        const Move user = get_user_move();
        const Move comp = get_random_move();

        countdown();    

        cout << "User: " << user << endl;
        cout << "Computer: " << comp << endl << endl;

        sleep(1);
        switch(get_winner(user,comp))
        {
            case 0: cout << "Tie!" << endl; break;
            case 1: cout << "User wins, " << user << " beats " << comp << endl; break;
            case 2: cout << "Computer wins, " << comp << " beats " << user << endl; break;
        }

        sleep(1);   

        /* Prompt the user to play another round.
         * Any input besides "yes","Yes","y", or "Y"
         * will break out of the main loop. */  
  // Note from Josay : the comment just above is probably wrong
        string again;
        cout << endl << "Enter Y to Play Again" << endl;
        cin >> again;   
        if (again == "Y") {}
        else break;

        cout << endl;
    }           
    return 0;       
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Surely the srand call in the get_random_move function is wrong. You should call srand once, at the beginning of main. \$\endgroup\$ – Bakuriu Jul 7 '13 at 5:39
  • 1
    \$\begingroup\$ Your output operator. The parameter m is not used. There is no return statement. \$\endgroup\$ – Martin York Jul 7 '13 at 5:46
2
\$\begingroup\$
  • Try not to use using namespace std. Using it in local scope (such as a function) is better than having it in global scope, but it's still quite repetitive to have it in multiple ones.

  • Only call std::srand() once in main(), preferably at the very top, otherwise your random values will always be the same.

  • You can greatly simplify get_comp() by having an enum for the moves and returning one based on an acquired random number:

    enum Move { ROCK, PAPER, SCISSORS };
    
    // ...
    
    Move get_comp()
    {
        return std::rand() % 2;
    }
    
  • Your loop in main() makes little sense. Here's an alternative:

    do
    {
        // play the game...
    
        std::cout << "Enter 'Y' to play again: ";
        char again;
        std::cin >> again;
    } while (toupper(again) == 'Y');
    
| 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.