7
\$\begingroup\$

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;
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Unfortunate that they didn't ask you to code the much more advanced version of the game that adds Lizard and Spock. Perhaps you can recommend that for your next project? \$\endgroup\$ Jan 14, 2023 at 4:47
  • 3
    \$\begingroup\$ While probably against the spirit of the game, it's worth noting that the behaviour is identical if you don't bother determining the computer's choice, and just randomly select between the won, lost and draw messages. \$\endgroup\$ Jan 14, 2023 at 13:36
  • \$\begingroup\$ For bonus points: since the computer makes its choice after the player, bias it to cheat some of the time... \$\endgroup\$ Jan 17, 2023 at 16:20
  • 2
    \$\begingroup\$ I have rolled back Rev 5 → 4. Please see What to do when someone answers. \$\endgroup\$ Jan 30, 2023 at 23:28

5 Answers 5

4
\$\begingroup\$
  • 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;
}
\$\endgroup\$
5
  • 1
    \$\begingroup\$ "move the srand to the top, no reason to call it multiple times" --> There is a reason - perhaps not commonly considered. srand(time(NULL)); forms only UINT_MAX + 1 different sequences. By calling srand(time(NULL)); every so often after an intervening scanf(), code injects another variation resulting in additional overall sequences. Your point is valid, yet the additional calls to srand() do pose some benefit in increasing variation. \$\endgroup\$ Jan 13, 2023 at 21:09
  • 2
    \$\begingroup\$ @chux-ReinstateMonica but for playing rock, paper, scissors (3 options) UINT_MAX+1 number of seq is plenty of times (unless someone is going to play millions of rounds), I'm aware of PRNG cyclic (and other) issues but for such a small program there isn't really a reason to call it multiple times \$\endgroup\$
    – depperm
    Jan 13, 2023 at 21:21
  • 2
    \$\begingroup\$ depperm, Agreed for a learner program, yet the idea does not certainly apply to larger tasks. So worth mentioning here as "no reason to call it multiple times" is not a universal axiom. \$\endgroup\$ Jan 13, 2023 at 21:28
  • 1
    \$\begingroup\$ Sorry, I don't see the relevance. We can choose from the rand() LCG, the oft berated Mersenne Twister, PCGs, and slower alternatives such as SHA3(key + counter) offered by our crypto friends. Both the K bits of state, and whether a sequence of 2**K unique outputs can be achieved, are of interest. In a physics code for ideal gas, we may want K large. Usually sharding distinct sequences across hosts is the sticking point. Occasional reseeds makes sense but exceeding the DRNG's limited rate of entropy production would be unfortunate. \$\endgroup\$
    – J_H
    Jan 13, 2023 at 22:48
  • 1
    \$\begingroup\$ @J_H I'm with you, assuming you're responding to chux. If you are generating millions of random numbers, you probably shouldn't be using rand(). \$\endgroup\$ Jan 14, 2023 at 14:30
9
\$\begingroup\$
    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!

\$\endgroup\$
4
  • 2
    \$\begingroup\$ "Consider preferring atoi()." --> even better, to avoid UB on error, use strtol(). \$\endgroup\$ Jan 13, 2023 at 21:12
  • \$\begingroup\$ Note : /dev/urandom is not part of standard C - yet a good choice when available. See also. \$\endgroup\$ Jan 13, 2023 at 21:13
  • \$\begingroup\$ The OP edited their answer about 14 seconds before your answer was submitted, correcting the typo regarding the escape character, and then left a comment in response to Ted's comment: "Sorry, it was a spelling mistake I accidentally introduced while translating xD" \$\endgroup\$ Jan 13, 2023 at 22:00
  • 2
    \$\begingroup\$ atoi() is awful whenever 0 is a valid input; learn to use the strtol() family for much better error handling. \$\endgroup\$ Jan 17, 2023 at 16:16
6
\$\begingroup\$

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) {
\$\endgroup\$
4
\$\begingroup\$

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).
\$\endgroup\$
1
  • \$\begingroup\$ while(getc(stdin) != '\n') {} is an infinite loop when getc(stdin) returns EOF. \$\endgroup\$ Jan 30, 2023 at 23:41
3
\$\begingroup\$

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.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ I considered answering this too, but as OP is asking for optimizations and writing in "plain C", I assumed an embedded development. In that case, this code is small enough that leaving out functions and keeping everything in one function may be the best option. \$\endgroup\$ Jan 15, 2023 at 8:38
  • \$\begingroup\$ @DaniëlvandenBerg Since when are functions not "plain C"? \$\endgroup\$ Jan 15, 2023 at 8:43
  • \$\begingroup\$ not what I said. I said plain C indicates embedded to me. That, in turn, could indicate that functions may not be desired. \$\endgroup\$ Jan 16, 2023 at 6:00
  • \$\begingroup\$ @DaniëlvandenBerg Firstly, it's certainly "plan C" because it's an exercise (presumably for a student), and secondly, it's a very limited embedded platform if it doesn't have a stack. Not unheard of, of course, but I'd be more concerned about the printf statements using the heap. \$\endgroup\$ Jan 16, 2023 at 7:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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