Rock, Paper, Scissors game with CPU

Question

I have written the below shared code and wanted to ask for some optimization tips or even alternative (more elegant?) ways of solving the task at hand (maybe without gotos?). My code, even if 'crude', should work properly.

The exercise basically asks me to recreate a Rock, Paper, Scissors game with the CPU (randomizing the CPU's moves) and to print a summary of our interaction.

//Rock, Paper, Scissors

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int main(void)
{
    //Variables Definition
    int user_choice, computer_choice, rematch = 0;  //Normal Variables
    int games_number = 0, games_played = 0, user_wins = 0, computer_wins = 0, even_games = 0;   //Counters

    start:
    //User Interface
    printf("How many matches (to the best of 1) would you like to play?\n");
    scanf("%d", &games_number);

    //Input Management
    if(games_number >= 1){
    for(int i = 0; i < games_number; ++i){  //For cycle (match) beginning
        srand(time(NULL));  //Computer choice randomization
        games_played += 1;  //Matches played counter increment

        //Request of chosen move
        printf("\nChoose your own move:\n1 = Rock\n2 = Paper\n3 = Scissors\n\n");
        scanf("%d", &user_choice);

        //Invalid input case
        if(user_choice <= 0 || user_choice >= 4){
            printf("\nYou have chosen an invalid move!\n(An invalid number was inserted)\n");
            goto end;
        }

        //Computer move simulation
        computer_choice = 1 + rand() % 3;
        printf("(Computer's choice': %d)\n", computer_choice);  //printf not required

        //Results computation
        if(user_choice == computer_choice){                     //user == pc
            printf("\nYou are even!\n");
            even_games += 1;
        }else if(user_choice == 1 && computer_choice == 2){     //user: rock | pc: paper
            printf("\nThe user has won, congratulations!\n");
            user_wins += 1;
        }else if(user_choice == 1 && computer_choice == 3){     //user: rock | pc: scissors
            printf("\nThe user has lost :(\n");
            computer_wins += 1;
        }else if(user_choice == 2 && computer_choice == 1){     //user: paper | pc: rock
            printf("\nThe user has won, congratulations!\n");
            user_wins += 1;
        }else if(user_choice == 2 && computer_choice == 3){     //user: paper | pc: scissors
            printf("\nThe user has lost :(\n");
            computer_wins += 1;
        }else if(user_choice == 3 && computer_choice == 1){     //user: scissors | pc: rock
            printf("\nThe user has lost :(\n");
            computer_wins += 1;
        }else if(user_choice == 3 && computer_choice == 2){     //user: scissors | pc: paper
            printf("\nThe user has won, congratulations!\n");
            user_wins += 1;
        }
    }//for cycle (match) end

    end:
    //Rematch request
    printf("\nDo you want to play again?\nYes: 1 || No: other\n");
    scanf("%d", &rematch);

    if(rematch == 1){
        puts("");
        goto start;
    }

    //Print summary
    printf("\n| %s | %s | %s | %s |\n", "Matches Played", "User Victories", "Computer Victories", "Even Matches");
    printf("| %14d | %14d | %17d | %12d |\n", games_played, user_wins, computer_wins, even_games);
    }else{  //if (games_number >= 1) end
        printf("\nAlright, go play something else then   :'(\n");
    }

    return 0;
}

Answer 1

• move the srand to the top, no reason to call it multiple times
• Get rid of the goto and instead rely an outer while loop and inner for loop (which you already have).
  • First loop is an endless loop while(1) that asks how many matches, if <=0 then exit the loop break. Add games_played to games_number so that if you finish one round and play again the count is total not just the last round
  • Next loop is for, this is the main loop
• In the main loop after asking for input if it's invalid restart the loop with continue and decrement i so player plays the correct number of rounds
• You could make the output of the computer choice more readable by adding a list of actions char actions[3][8]={"rock","paper","scissors"} and then output the corresponding action instead of the number printf("(Computer's choice': %s)\n", actions[computer_choice-1]);
• I'd consolidate the win/lose/even into 3 if/else, if there is a tie, else if the computer wins, or if the player wins
• increment games played last after playing, not at the beginning
• If the player chooses not to replay you can exit with break in the outer loop
• Print the stats if games_played > 0 (also minor spacing issue should be 18 not 17)

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

int main(void)
{
    //Variables Definition
    char actions[3][8]={"rock","paper","scissors"};
    int user_choice, computer_choice, rematch = 0;  //Normal Variables
    int games_number = 0, games_played = 0, user_wins = 0, computer_wins = 0, even_games = 0;   //Counters
    srand(time(NULL));  //Computer choice randomization

    while(1){
      //User Interface
      printf("How many matches (to the best of 1) would you like to play?\n");
      scanf("%d", &games_number);
      if(games_number<=0){
        printf("\nAlright, go play something else then   :'(\n");
        break;
      }
      for(int i = 0; i < games_number; ++i){
        printf("Choose your own move:\n1 = Rock\n2 = Paper\n3 = Scissors\n\n");
        scanf("%d", &user_choice);

        //Invalid input case
        if(user_choice <= 0 || user_choice >= 4){
            printf("\nYou have chosen an invalid move!\n(An invalid number was inserted)\n");
            i -= 1; // because you are asking user input again but don't want to increment round
            continue;
        }
        computer_choice = 1 + rand() % 3;
        printf("(Computer's choice': %s)\n", actions[computer_choice-1]); 

        if(user_choice == computer_choice){                     //user == pc
            printf("\nYou are even!\n");
            even_games += 1;
        } else if((user_choice < computer_choice)||
          (user_choice == 3 && computer_choice == 1)){
          printf("\nThe user has lost :(\n");
          computer_wins += 1;
        }else{
          printf("\nThe user has won, congratulations!\n");
          user_wins += 1;
        }
        games_played += 1;
      }
      printf("\nDo you want to play again?\n 1: Yes || Other: No\n");
      scanf("%d", &rematch);
    
      if(rematch != 1){
        break;
      }
    }
    if (games_played>0){
      //Print summary
      printf("\n| %s | %s | %s | %s |\n", "Matches Played", "User Victories", "Computer Victories", "Even Matches");
      printf("| %14d | %14d | %18d | %12d |\n", games_played, user_wins, computer_wins, even_games);
    }

    return 0;
}

Answer 2

    int games_number = 0, games_played = 0, user_wins = 0, computer_wins = 0, even_games = 0;   //Counters

Wow, that's a lot of counters! When choosing a data representation, think about whether there is redundant information. It's not always bad, but it does open the door to inconsistencies. And then code has to decide "which one is right?" Sometimes different pieces of code will make different conflicting decisions.

Here, the games_played should probably be re-computed each time as a sum of other counters.

---

    scanf("%d", &games_number);

Gosh, I didn't know people still use scanf in new code. I guess this is nice enough, since it's not a %s ? But it's not a habit I would care to encourage. Consider preferring atoi(). Buffer overruns are no fun.

---

        printf("\Choose your own ...

As @TedLyngmo observes, you should definitely remove that \ backwhack.

---

There's nothing particularly wrong with your goto end;. It's a standard idiom for bailing out, especially in code that has to cleanup resources on the way out. It saves a level of if indent for the bulk of your code.

The goto start; is pretty odd -- a do / while loop should be bringing us back up there.

What the gotos highlight is that this function is starting to become a bit inconveniently long. Ideally we'd like each function to have a well defined API which does one thing, and its source should be visible within a single editor window with no scrolling (at default font size). So you get maybe fifty or sixtyish source lines.

Consider banishing some of the prompting / validating to three-line helper functions. Adjudicating the winner definitely belongs in its own function.

---

    for ( ...
        srand(time(NULL));  //Computer choice randomization

I wouldn't get too carried away with re-seeding. Doing it once, when the program starts, is enough.

Or if you do repeatedly re-seed, seed with something random, bytes you read from /dev/urandom. Intel went to a lot of trouble to put a DRNG into Ivy Bridge. May as well use that source of entropy, rather than deterministic wall-clock time.

Notice that your timestamp has one-second resolution. So if a round of play is completed in 900 msec and we come back for more, we may be deterministically telling the PRNG to generate the identical moves, which hardly seems sporting. Further, we assume an attacker has access to approximately the same timestamps, and could "predict" moves with perfect accuracy as long as he can observe live play. In the end, there's little substitute for incorporating some source of unobservable entropy.

---

        // Results computation
        if (user_choice == computer_choice) {                     //user == pc
            printf("\nYou are even!\n");
            even_games += 1;
        } else if (user_choice == 1 && computer_choice == 2) {     //user: rock | pc: paper
            printf("\nThe user has won, congratulations!\n");
            user_wins += 1;
        } else if (user_choice == 1 && computer_choice == 3) {     //user: rock | pc: scissors

DRY. When you're writing code, look for opportunities to take a chunk of code and push it down into some tiny isolated helper function. This is the perfect opportunity. Define a public API that says we accept a pair of choices and we adjudicate the winner. Call that helper function here.

Also, one way of encoding such decisions is to store all the possibilities in an array and then return an array lookup value.

---

Seems like a good initial effort!

Answer 3

Many points already covered.

Check input functions return values

// scanf("%d", &rematch);
if (scanf("%d", &rematch) != 1) {
  Handle_input_error(): //TBD code
}

Output alignment

Use a width for easier adjustments.

// printf("\n| %s | %s | %s | %s |\n", 
//     "Matches Played", "User Victories", "Computer Victories", "Even Matches");
// Example
printf("\n| %14s | %14s | %17s | %-12s |\n", 
    "Matches Played", "User Victories", "Computer Victories", "Even Matches");

printf("| %14d | %14d | %17d | %12d |\n", 
    games_played, user_wins, computer_wins, even_games);

Or maybe drive from auxiliary values.

// example, a uniform width
#define COL_N 17

printf("\n| %*s | %*s | %*s | %-*s |\n", 
  COL_N, "Matches Played", COL_N, "User Victories", //
  COL_N, "Computer Victories", COL_N, "Even Matches");

printf("| %*d | %*d | %*d | %*d |\n", //
  COL_N, games_played, COL_N, user_wins, //
  COL_N, computer_wins, COL_N, even_games);

Style

When checking limits, consider comparing against the minimum and maximum, rather than the minimum - 1 and maximum + 1.

In general, minimum and maximum exist, but not certainly minimum - 1 and maximum + 1. Comparing against the min/max is often clearer.

// if(user_choice <= 0 || user_choice >= 4){
if(user_choice < 1 || user_choice > 3) {
// or 
#define CHOICE_MIN 1
#define CHOICE_MAX 3
if(user_choice < CHOICE_MIN || user_choice > CHOICE_MAX) {

Answer 4

When it comes to code structure, here are some tips that may be helpful:

1. Don't use gotos when you can achieve the same behaviour with for/while loops.
2. The variable games_played can be calculated with user_wins+computer_wins+even_games, so you can save memory here.
3. Divide big code into multiple functions. I added the function playGame to increase readability.
4. Connect multiple if-conditions to one with the OR operator || to not repeat code.

I rewrote your code to the following:

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

/*
-1: Invalid input
0: Even
1: User wins
2: Computer wins
*/
int playGame() {
    srand(time(NULL));  //Computer choice randomization
    int computer_choice = 1+ rand() % 3;
    int user_choice;

    //Request of chosen move
    printf("\nChoose your own move:\n1 = Rock\n2 = Paper\n3 = Scissors\n\n");

    //Invalid input case
    int failure = scanf("%d", &user_choice) <= 0;
    if(failure || user_choice <= 0 || user_choice >= 4) {
        if(failure) {
            // When scanf fails, its not reading the whole input, causing other scanf calls behave unexpected. This is a quick fix.
            int in;
            do {
               in = getc(stdin);
            } while(in != '\n' && in != EOF);
        }
        printf("\nYou have chosen an invalid move!\n(An invalid number was inserted)\n");
        return -1;
    }

    //Computer move simulation
    printf("(Computer's choice': %d)\n", computer_choice);  //printf not required

    if(user_choice == computer_choice) {
        return 0;
    }
    if((user_choice == 1 && computer_choice == 2) || //user: rock | pc: paper
        (user_choice == 2 && computer_choice == 1) || //user: paper | pc: rock
        (user_choice == 3 && computer_choice == 2)) {//user: scissors | pc: paper
        return 1;
    }
    return 2;
}

int main() {
    //Variables Definition
    int games_number; // Input
    int user_wins = 0, computer_wins = 0, even_games = 0;   //Counters

    while(true) {
        printf("How many matches (to the best of 1) would you like to play?\n");

        if(scanf("%d", &games_number) <= 0 || games_number <= 0) {
            // Cases: No Number, Number = 0, EOF
            printf("\nAlright, go play something else then   :'(\n");
            return 0;
        }

        while(games_number--) {
            int matchresult = playGame();
            if(matchresult == -1) { // Invalid input
                break;
            } else if(matchresult == 0) {
                printf("\nYou are even!\n");
                even_games++;
            } else if(matchresult == 1) {
                printf("\nThe user has won, congratulations!\n");
                user_wins++;
            } else {
                printf("\nThe user has lost :(\n");
                computer_wins++;
            }
        }

        int rematch = 0;
        printf("\nDo you want to play again?\nYes: 1 || No: other\n");

        if(scanf("%d", &rematch) <= 0 || rematch != 1) {
            printf("\n| %s | %s | %s | %s |\n", "Matches Played", "User Victories", "Computer Victories", "Even Matches");
            printf("| %14d | %14d | %17d | %12d |\n", user_wins+computer_wins+even_games, user_wins, computer_wins, even_games);
            break;
        }

        puts("");
    }
}

As you can see, I

• set some of the variable declarations within the code to signal that the variable is not used before the declaration,
• used uninitalized variables that will be initalized for sure later
• replaced the goto statements with while loops and break/return statements,
• considered the scanf function to fail and checked for its return value, and
• fixed a bug in your code (quick fix in playGame function).

Answer 5

Use functions

Abstract away the user choice selection, and the CPU choice selection into two different functions (with the same signature).

Theoretically this would allow you to make the CPU play itself, or two human players play each other.

It's getting a bit advanced for your level, but in the future, you would look at function pointers etc.