11
\$\begingroup\$

Just started learning C. I hope my code adheres to best practices and idioms

#include <stdio.h> #include <ctype.h> #include <time.h> #include <stdlib.h> int main(void) { int iRandomNum; int iGuess; srand(time(NULL)); iRandomNum = (rand() % 10); printf("Guess a number between 1 and 10:"); scanf("%d", &iGuess); // isdigit function expects a character so the guess value has to be ascii value of the digit character iGuess += 48; if (isdigit(iGuess)){ iGuess -=48; // change back to the original value for comparison with random number if (iGuess == iRandomNum){ printf("You Guessed Correctly\n"); } else{ printf("The correct answer was %d\n",iRandomNum); printf("You guessed %d\n",iGuess); } } else{ printf("This is not a digit!\n"); } }
\$\endgroup\$

3 Answers 3

Reset to default
13
\$\begingroup\$

I have observed a few things that may help you improve your code.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Check return values for errors

The call to scanf can fail. You can check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources. Rigorous error handling is the difference between mostly working versus bug-free software. You should strive for the latter.

Don't use Hungarian notation

Prefixing every variable with an abbreviation of its type is usually called "Hungarian notation" and it was once popular. Even then it was a bad idea. Don't clutter up your source code with that; instead, concentrate on defining meaningful names for each variable and choose types appropriately. I'd suggest secretNumber and userGuess in this case.

Use a better random number generator

You are currently using 

iRandomNum = (rand() % 10);

There are a number of problems with this approach. The most significant problem is that the range this returns (0 through 9 inclusive) does not match what the user is asked to guess ("a number between 1 and 10") Second, this will generate lower numbers more often than higher ones -- it's not a uniform distribution. Another problem is that the low order bits of the random number generator are not particularly random, so neither is the result. On my machine, there's a slight but measurable bias toward 0 with that. See this answer for details, but I'd recommend changing that to 

secretNumber = rand() / (RAND_MAX / 10) + 1;

Think of the user

When someone asks me to "Guess a number between 1 and 10", I usually say \$e\$. (Yes, I'm afraid I'm that kind of person.) It seems clear from the context of your program that what you really mean is to guess an integer, and it seems likely that, technically speaking, you don't want numbers solely between 1 and 10 but that you mean to include 1 and 10 among the possible choices. To more clearly convey that, I'd suggest asking the user to "guess a whole number from 1 to 10 inclusive" but I'm probably too used to being around engineers and scientists.

Check your logic

If I input "e" as my guess, the scanf call will fail and the variable may be uninitialized.

\$\endgroup\$
2
  • \$\begingroup\$ Tangential question from a fellow scientist: I always go with pi instead of e. Have you found advantages to either? \$\endgroup\$ Commented Feb 25, 2017 at 1:05
  • 6
    \$\begingroup\$ That's a very insightful question. In fact, I find that the advantage to choosing e over pi is that fewer people seem to have heard of e than pi, so choosing e yields dual advantages: 1) I can hold forth for untold hours expounding on the genius of Euler, the sublime beauty of Euler's identity, the constant e, natural logarithms and why all of it matters deeply to modern society and 2) nobody ever asks me again to pick a number. \$\endgroup\$ Commented Feb 25, 2017 at 3:21
7
\$\begingroup\$

As Edward said:

Pick a style and apply it consistently.

As with normal text, people will get discouraged working with you and giving feedback if the text you have written is messy.

Imagine:

az wi Normal teXt pepls ~~ DiscourageD w0rking wi you ++giving feedback if zee teXT y heff vritten is mezzy.

Here is one as an example of a style (from an introductory CS course). There should be an updated version here soon.

I am also learning C and tried to make your code consistent to the aforementioned style.

Is it not easier on the eye now?

/** * Guessing Game * Here goes some description... * ..... */ #include <stdio.h> #include <ctype.h> #include <time.h> #include <stdlib.h> int main(void) { // Initalize variables int random_num; int guess; // Add comment here srand(time(NULL)); random_num = (rand() % 10); // Add comment here printf("Guess a number between 1 and 10:"); scanf("%d", &guess); // isdigit function expects a character so the guess value has to be // ascii value of the digit character guess += 48; if (isdigit(guess)) { // change to the orig value and compare with random_num guess -= 48; // Add comment here if (guess == random_num) { printf("You Guessed Correctly\n"); } else { printf("The correct answer was %d\n", random_num); printf("You guessed %d\n", guess); } } else { printf("This is not a digit!\n"); } }
\$\endgroup\$
5
\$\begingroup\$

Write what you mean

guess += 48; if (isdigit(guess)) { guess -= 48;

Not only could the be simplified to if (isdigit(guess + 48)) { but it obfuscates two things:

  • That you want to add the ASCII code of the 0 character. Use '0' instead of 48 here.

  • That you actually want to check if guess lies between 0 and 9 which you can do much more concisely and readably with:

    if (guess >= 0 && guess <= 9) {

This is equivalent (see below) to the previous check but inconsistent with the requested number range „between 1 and 10“.

Avoid the invocation of undefined behaviour

The manual page of isdigit(3) says that the input value of the function

must have the value of an unsigned char or EOF […].

In your program the user of your program enters a number, scanf stores it in the variable guess which is later handed to isdigit. If guess is outside of the range specified above, the program behaviour is undefined. In theory that means the function may perform arbitrary operations for these cases. In practice it means that you can't rely on its return value in these cases.

As an example, the specification in the manual allows the implementation of isdigit to truncate its input value to the least significant 8 bits (after checking for EOF) and looks up the character class in a table of length 256 (which is a sensible and realistic implementation because there are 256 different unsigned char values); in that case an input value 256 would be transformed and then truncated to (256 + 48) % 256 = 48 which is the ASCII code of a digit character even though 256 is not between 0 and 9.

\$\endgroup\$
1
  • \$\begingroup\$ Excellent point about isdigit. Good catch! \$\endgroup\$ Commented Feb 25, 2017 at 14:58

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.