5
\$\begingroup\$

I was given for homework to make a rock paper scissor game where I input my choice as a string either: "@" for rock, "[]" for paper or "X" for scissors. The computer chooses a random one and it's printed who wins. I'm worried that my code isn't good enough.

#include <stdio.h>      /* printf, scanf, NULL  */
#include <stdlib.h>     /* srand, rand          */
#include <time.h>       /* time                 */
#include <string.h>     /* strcmp               */

/* Configure */
#define MAX_SIGN_LEN    (8)
#define STR_ROCK        "@"
#define STR_PAPER       "[]"
#define STR_SCISSORS    "X"

typedef enum RPS_ENUMERATOR { ROCK,         PAPER,      SCISSORS,   N_RPS       } rps;
typedef enum RPS_RESULT     { YOU_LOOSE,    YOU_WIN,    TIED,       N_RESULTS   } rpsResult;

const char *rpsSigns    [N_RPS]     = { STR_ROCK,       STR_PAPER,  STR_SCISSORS };
const char *gameResult  [N_RESULTS] = { "YOU LOOSE!",   "YOU_WIN",  "TIED" };

rpsResult combinations[N_RPS][N_RPS] =
{
    {TIED,      YOU_WIN,    YOU_LOOSE},
    {YOU_LOOSE, TIED,       YOU_WIN},
    {YOU_WIN,   YOU_LOOSE,  TIED}
};


int validate_answer (char buffer[MAX_SIGN_LEN], rps rpsUser)
{
    if(rpsUser == N_RPS)
    {
        printf("WTF is \"%s\" ?\n", buffer);
        return 1;
    }

    return 0;
}

rps get_choice_value (char buffer[MAX_SIGN_LEN])
{
    rps rpsUser = ROCK;

    for(rpsUser = ROCK; rpsUser < N_RPS; rpsUser++)
        if(!strcmp(buffer, rpsSigns[rpsUser]))
            break;

    return rpsUser;
}

void run_game ()
{
    rps     rpsRandom               = ROCK;
    rps     rpsUser                 = ROCK;
    char    buffer[MAX_SIGN_LEN]    = {'\0'};

    /* Generate a random number between 0 and 3 (inclusive-exclusive) */
    rpsRandom = rand() % N_RPS;

    /* Print requirement */
    printf("Choose rock, Paper, Scissors ( %s | %s | %s ): ", rpsSigns[ROCK], rpsSigns[PAPER], rpsSigns[SCISSORS]);

    /* Ask user */
    scanf("%8s", buffer);

    /* Obtain user choice as a number */
    rpsUser = get_choice_value(buffer);

    /* Check if choice was valid */
    if(validate_answer(buffer, rpsUser) == 1)
        return;

    /* Print result */
    printf("Program chose: %s. %s\n", rpsSigns[rpsRandom], gameResult[combinations[rpsRandom][rpsUser]]);
}

int main (void)
{
    /* initialize random seed: */
    srand(time(NULL));

    /* Run game */
    while(1) run_game();

    return 0;
}
\$\endgroup\$
5
  • 7
    \$\begingroup\$ I know it's just homework, but the opposite of "win" is "lose", not "loose" (which is the opposite of "tight"). Just one letter apart, but totally different words. \$\endgroup\$ Nov 9, 2022 at 13:13
  • 1
    \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$
    – pacmaninbw
    Nov 12, 2022 at 17:59
  • \$\begingroup\$ @pacmaninbw Are you serious? He literally just fixed a grammar mistake that is only mentioned in the comment section. \$\endgroup\$
    – Edenia
    Nov 12, 2022 at 18:05
  • \$\begingroup\$ @Edenia Your first paragraph mentions line length, the edit reduced the line length. \$\endgroup\$
    – pacmaninbw
    Nov 12, 2022 at 22:20
  • \$\begingroup\$ @pacmaninbw Ah that's fair point :) At least it's not significant. \$\endgroup\$
    – Edenia
    Nov 12, 2022 at 22:21

2 Answers 2

8
\$\begingroup\$

Readability

As mentioned, the width of the code spans through 112 characters, which is /not/ obviously too far. I recommend it not exceed 100 characters at most. Even that can be a little too much and force people to do a lot of eye movement in order to read your code. Consider:

printf("Choose rock, Paper, Scissors ( %s | %s | %s ): ",
       rpsSigns[ROCK], rpsSigns[PAPER], rpsSigns[SCISSORS]);

or

#define ENTRY_TEXT "Choose rock, Paper, Scissors ( %s | %s | %s ): "
...
printf(ENTRY_TEXT, rpsSigns[ROCK], rpsSigns[PAPER], rpsSigns[SCISSORS]);

Overflow

The other valid point that was already mentioned is that scanf is called to read up to 8 characters, but it also transparently writes '\0' at the end of buffer so it may end up writing 9 characters and cause a buffer overflow. Change the format parameter to "%7s" (as chux said, 7 being the specifier width)

Error-checking

I would recommend you to check scanf against the number of items of the argument list successfully read. Not a bad practice.

Limit restrictions

It is perfectly fine to hard code length limits, but if you do so, make sure you let the user be aware, perhaps by printing at the end of input suggestion "(max 8 characters)" Otherwise Users might be puzzled as to why their input has been abruptly trimmed.

Infloops

I can understand why are you taking a serving of the while(1) loop. Loops without an escape clause are nothing more than intended deadlocks. Intended or not, they are still deadlocks and should be taken care of. Minimal changes can be made to force your way out of the loop if you type "exit" or just "e" or maybe do that after 100 rounds. At the end, you can print statistics of how many rounds were lost, won or tied.

Implementation details

control

Minimal changes can be made to allow the user to specify how they want the rock, paper, scissors -representing strings to look like. One would prefer singular characters such as "R", "P", "S", other might prefer morse code...

consider using fgets()

fgets() is a simpler function that will read the entire line and will not stop on just about any whitespace (which can be more or less hideously achieved via %7[^\n]%c"...)

if(fgets(buffer, MAX_SIGN_LEN, stdin) == NULL)
    puts("Treat error...");

Thanks to @Toby Speight for reminding me that gets is unsafe as it doesn't have any means to protect against possible overflow

return value

If your program loop now has an escape clause, you can probably return errno or something. If the improved program design follows the 100 turns advice you can return EXIT_FAILURE if for some reason not all games were played out.

\$\endgroup\$
6
  • \$\begingroup\$ All good points, though I'd probably recommend static const char *const entry_text = "…:; as better than defining a macro. And gets() is a notorious function that cannot be used safely - use fgets() instead. \$\endgroup\$ Nov 9, 2022 at 13:12
  • 1
    \$\begingroup\$ @TobySpeight Ahh yes! I was thinking about fgets at the beginning, but for some reason wrote gets when I got to that part. Thank you! (Though, I will edit that part now, because it's an awful advice) \$\endgroup\$
    – Edenia
    Nov 9, 2022 at 14:49
  • \$\begingroup\$ A buffer overflow is most certainly not a stack overflow, though both are bugs. \$\endgroup\$ Nov 9, 2022 at 17:29
  • \$\begingroup\$ Yes, buffer overflow is far more likely :D Oh wait.. \$\endgroup\$
    – Edenia
    Nov 9, 2022 at 18:11
  • \$\begingroup\$ <Change the format parameter to "7"> and <Change the specifier width to "7"> emphasizes what the 7 is about. \$\endgroup\$ Nov 9, 2022 at 22:58
6
\$\begingroup\$
  1. Try to limit the line-width. Horizontal scrolling is death on readability.

  2. Commenting after includes what symbols you need it for is quite uncommon, as it just clutters things up and those comments easily become stale.
    The exception are raw beginners who might do it to ensure they have it all covered, and don't have superfluous includes either, so might be justified as an exercise.

  3. If you have a typedef-name and a tag-name, use the same for both. Remember that any readers brain-space isn't perfectly separated like C's namespaces.

  4. There is a very strong convention that FULY_CAPITALIZED names are macros. There can be some additional macros (especially function-style macros, whether they shadow an actual function or not, as long as they work like true functions), but there shouldn't be many.
    Don't break this convention without very good reason.

  5. If you provide initializers for all array elements, specifying array-bounds is needless duplication.
    Simplify and leave them out, or add a static assert to properly nail them down.

  6. You can recover the element-count from an array using sizeof array / sizeof *array. This does not work for pointers to the first element, like after array-decay as in function-arguments. Best hide behind a macro #define COUNT(array) (sizeof array / sizeof *array).

  7. Try to avoid scattering the same (or derived) info all over. Single-source-of-truth is very useful for correctness, flexibility, simplicity, conciseness, and readability, all crucial for maintainability.

  8. If a function parameter looks like an array (of specified bounds), it's actually a pointer. Only make it look like an array to document the parameter will be used as such, and only give a length exactly that many elements will always be accessed.

  9. If scanf() consumes 8 input-characters, it will write 9 characters including the 0-terminator. Thus, you have a buffer-overflow, sub-category fence-post-error.

  10. return 0; is implicit for main() since C99. Make of that what you will. Anyway, that's dead code.

\$\endgroup\$
10
  • 2
    \$\begingroup\$ "There is a very strong convention that all macros " --> it is not that strong. Even the C spec breaks it often. Best to follows group's coding standards. \$\endgroup\$ Nov 9, 2022 at 23:00
  • 1
    \$\begingroup\$ "... and don't have superfluous includes " ---> I find an occasional unneeded #include in a .c file not worth commenting. The minimal set of includes is best handled with software automation and not valuable coder's time. Apparent unneeded includes in a .c file can exist to insure collisions with the standard library names do not occur. OTOH, in a .h file, eliminating unneeded #include is a best practice. \$\endgroup\$ Nov 9, 2022 at 23:06
  • 2
    \$\begingroup\$ "specifying array-bounds is needless duplication." --> Hmm. From time to time that duplication is very useful, especially with large arrays of a spec'd size. \$\endgroup\$ Nov 9, 2022 at 23:08
  • \$\begingroup\$ Overall , a good review. \$\endgroup\$ Nov 9, 2022 at 23:09
  • \$\begingroup\$ @chux-ReinstateMonica Specifying array bounds is a very helpful correctness check whrn you accidentally put in too many elements. When you put in too few, though, compilers will pad the array out with default values and not give you so much as a warning. So, not as useful as it could be. \$\endgroup\$
    – Davislor
    Nov 9, 2022 at 23:39

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.