Simple text based game in C

Question

After sticking with the same handful of languages for years, I decided to learn C.

I've written a guess the number game, where it generates a random number based on the difficulty level, and then you have 5 attempts of guessing the number.

If you guess incorrect, it will give you a clue by telling you if the answer is higher or lower than your last guess. I find the game to be pretty interactive, and wanted to get a code review to see how I could improve on it.

This is my first ever C program and I'm just looking to improve.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int generate_random_number(int min, int max) {
 srand ( time(NULL) );
 return min + (rand() % (max - min));
}
void play() {
 int difficulty = 1; // 1 = easy, 2 = medium, 3 = hard, 4 = insane
 printf("1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE\n");
 printf("What level would you like to play: ");
 scanf("%i", &difficulty);
 int min = 0;
 int max = 0;
 int random = 0;
 if (difficulty == 1) {
 min = 0;
 max = 25;
 printf("You have selected to play easy\n\n");
 }
 else if (difficulty == 2) {
 min = 0;
 max = 50;
 printf("You have selected to play medium\n\n");
 }
 else if (difficulty == 3) {
 min = 0;
 max = 75;
 printf("You have selected to play hard\n\n");
 }
 else if (difficulty == 4) {
 min = 0;
 max = 100;
 printf("You have selected to play insane\n\n");
 }
 random = generate_random_number(min, max);
 int tries = 5;
 int won = 0;
 while (tries > 0)
 {
 int guess = 1000000; // just so it doesn't accidentally equal to random
 printf("Guess a number %i to %i: ", min, max);
 scanf("%i", &guess);
 if (guess == random) {
 won = 1;
 break;
 }
 else {
 if (guess > random) {
 printf("Incorrect guess, the answer is lower than your guess!\n\n");
 }
 else {
 printf("Incorrect guess, the answer is higher than your guess!\n\n");
 }
 }
 tries -= 1;
 }
 if (won) {
 printf("Congratulations, you have won the game!");
 }
 else {
 printf("Sorry, you are out of tries.\n\n");
 }
}
int main() {
 while (1) {
 play();
 }
 return 0;
}

Answer 1

Good:

no warnings with -Wall -Wextra -pedantic with both gcc and clang, no memory leaks found with valgrind.

Bad:

use prototypes instead of declarations to give compiler a chance to issue warnings when an incorrect number of parameters is passed or incorrect types are passed:

void play(void)
int main(void)

You don't check if user passes correct acceptable difficulty level:

$ ./main
1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE
What level would you like to play: 6
Floating point exception
$ ./main
1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE
What level would you like to play: 0
Floating point exception

You don't check if user passes an integer in the first place:

$ ./main
1 = EASY, 2 = MEDIUM, 3 = HARD, & 4 = INSANE
What level would you like to play: a

will make your program go into an endless loop.

Suggested:

convert int difficulty = 1 into an en enum with additional DIFFICULTY_MAX and DIFFICULTY_MIN values, and check if value passed from the user is lower than DIFFICULTY_MAX and larger or equal to DIFFICULTY_MIN.

Use EXIT_SUCCESS to denote success at the end of main():

return EXIT_SUCCESS;

or just omit the return from main() - compiler will automatically return a success value if we run off the end.

• Josh Hallow: Thank you, I have marked this the correct answer as it was the one which helped me most.

Answer 2

int generate_random_number(int min, int max) {
 srand ( time(NULL) );
 return min + (rand() % (max - min));
}

You should only seed the random number generator once, at the start of main. In this case, if (for some reason) somebody played more than one round in a single second, both games would have the same number. Probably not a huge issue for this particular program, but something to be aware of.

• Toby Speight: "(for some reason)" could be an automated test, for example.
• Darrel Hoffman: @TobySpeight Not with time(NULL) as a seed - presumably that would change between one test and the next. If you wanted to do automated tests, you'd pick some constant as the seed and leave this line in here. But most likely N. Shead is correct that you would only want to do this once.
• Josh Hallow: Thanks for the response, when seeding random is the call to srand expensive, is this why I should only call it once?
• Toby Speight: @Ash - it's not that it's expensive, but because reseeding from time at (possibly) predictable intervals may perturb the randomness of the distribution. That's a minor concern in a game or simulation, but becomes very important when you're creating random secrets for cryptography.

Answer 3

if (difficulty == 1) {
else if (difficulty == 2) {
else if (difficulty == 3) {

This looks like switch (difficulty) might be more appropriate - perhaps with a default branch to catch out-of-range values.

Or, more simply, since we're just picking values, and min is always 0, just select from an array values (after verifying that the user's choice is in range):

int max[] = { 25, 50, 75, 100 };

In fact, with these values, we could simply multiply:

int max = 25 * difficulty;

---

scanf("%i", &difficulty);

Don't just discard the result from scanf() - always test that it converted as many values as you wanted. In this case,

if (scanf("%i", &difficulty) != 1) {
 fputs("Enter a number!\n", stderr);
 exit(EXIT_FAILURE); /* or some better handling */
}

Similarly here:

 scanf("%i", &guess);

---

Finally, it would be polite to allow users to exit the game when they get bored of it (I know, I can't quite believe that might happen, either!).

• LoztInSpace: Rather than a case statement, I'd have an array of structures with min, max and description then index to that (having first validated the choice).
• Toby Speight: Yes, that makes more sense