6
\$\begingroup\$

This is my first program in C. I'd rather not form any bad habits now. Is there anything that looks like bad practice, or something just looks wrong?

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

int main(void) {
    srand(time(NULL));
    int r = rand() % 10 + 1;
    int correct = 0; 
    int guess; 
    int counter = 0; 

    printf("Guess my number! "); 

    do {
        scanf("%d", &guess);
        if (guess == r) {
            counter++;
            printf("You guessed correctly in %d tries! Congratulations!\n", counter);
            correct = 1; 
        }

        if (guess < r) {
            counter++;
            printf("Your guess is too low. Guess again. ");
        }

        if (guess > r) { 
            counter++; 
            printf("Your guess is too high. Guess again. ");
        }
    } while (correct == 0);

    return 0;
}
\$\endgroup\$
7
\$\begingroup\$

Here are some of the improvements i can suggest:

  • move your counter variable , from the inside the if statements , as increasing the count is independent of the if condition.
  • Use srand(time(NULL)); , below the declarations , or else , your compiler may throw this warning , when compiled using C90 standard.

    warning: ISO C90 forbids mixed declarations and code [-pedantic]

    So to ensure portability , avoid statements between declarations.

  • In the long run , more meaningful and formal names , can be used instead of names like r and guess.

In my personal opinion , these two changes would be better:

  • Using a break; , when the correct number is guessed , can eliminate the need for using an extra variable correct.

  • I would prefer to use just a while() instead of do{}while();, if the program logic permits it.

Here is the program , modified with the changes , i listed above:

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

int main(void) 
{

int random_num = 0;
int guessed_num = 0;
int counter = 0; 

srand(time(NULL));
random_num = rand() % 10 + 1;

printf("Guess my number! "); 

    while(1)
    {
        counter++; 

        scanf("%d", &guessed_num);

        if (guessed_num == random_num) 
        {
            printf("You guessed correctly in %d tries! Congratulations!\n", counter); 
            break;
        }

        if (guessed_num < random_num) 
            printf("Your guess is too low. Guess again. ");

        if (guessed_num > random_num) 
            printf("Your guess is too high. Guess again. ");

    } 

return 0;   
}
\$\endgroup\$
  • \$\begingroup\$ The C90 warning is nothing to be concerned about, beginner programmers should only use the C99/C11 standards. \$\endgroup\$ – Lundin Mar 26 '13 at 9:38
  • 2
    \$\begingroup\$ I strongly disagree about using while(1) with a break instead of a loop that checks a flag variable. No matter, the comments about break and while are both your personal opinions about coding style. You need to state them as such, as there is no consensus among the whole software industry. On the contrary, several widely recognized programming standards don't share your opinion at all. Also, many would consider removing the {} after is as dangerous and bad style. Downvote until you have separated sound advise about good programming practice from personal opinions. \$\endgroup\$ – Lundin Mar 26 '13 at 9:41
  • \$\begingroup\$ Downvote changed to upvote :) \$\endgroup\$ – Lundin Mar 26 '13 at 9:52
3
\$\begingroup\$
  • You need to use proper indention of everything inside main().
  • Every scanf("%d") leaves a trailing line feed '\n' in stdin. They are skipped by further scanf("%d") reads, but if you attempted to scanf a character or string somewhere, you would get odd behavior. It is better to clear stdin after each access. The simplest way to do this is to add a call to getchar() after each scanf call. You can also start the scanf format string with a space, like scanf(" %s", str), the space will then discard left-over white spaces from stdin.
  • The if statements could be rewritten as if(guess == r) {...} else if(guess < r) {...} else {}, this will make the program slightly more effective since it prevents multiple, redundant checks of the same variable. If the guess == r then you don't need to check if it is less than r as well.
  • Instead of using an int variable correct with possible values 0 and 1, use a bool variable with values false and true (stdbool.h).
  • The counter++ should be removed from inside the if statements to the main body of the loop, as it is always increased no matter.

EDIT

Also, consider adding new line characters after each print, for readable output. Code with the above suggestions:

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

int main(void) {
  srand(time(NULL));

  int r = rand() % 10 + 1;
  bool correct = false; 
  int guess; 
  int counter = 0; 

  while(!correct)
  {
    printf("Guess my number! "); 
    scanf("%d", &guess);
    getchar();

    if (guess < r) {
        printf("Your guess is too low. Guess again.\n");
    }
    else if (guess > r) { 
        printf("Your guess is too high. Guess again.\n");
    }
    else /* if (guess == r) */ {
        printf("You guessed correctly in %d tries! Congratulations!\n", counter); 
        correct = true; 
    }

    counter++;
  } /* while(!correct) */

  return 0;   
}
\$\endgroup\$
  • \$\begingroup\$ I like this, but I agree with @BarathBushan about moving srand(time(NULL)) below the declarations. I like to keep declarations together so the body is only logic/function calls. \$\endgroup\$ – beatgammit Mar 27 '13 at 8:46
  • \$\begingroup\$ @tjameson It doesn't really matter where you put it. The remark in the other answer wasn't as much about style, as about compatibility with an obsolete version of the C standard. \$\endgroup\$ – Lundin Mar 27 '13 at 8:55
  • \$\begingroup\$ Right, which is why I qualified it with a style reasoning. But don't worry, +1 from me for a great answer. \$\endgroup\$ – beatgammit Mar 27 '13 at 9:01
0
\$\begingroup\$
/*
 * File: guessing_game_codereview.c
 * Purpose: The Guessing Game
 * Date: 2015-08-28
 * Author: Robert A. Nader
 * Email: naderra at g ...
 * Platform: Linux
 * Compile: gcc -std=c89 -Wall -Wpedantic \
                 guessing_game_codereview.c \
                 -o guessing_game_codereview
 * Note: Should be fairly portable to any hosted implementation.
 * Kept source file line lengths under 80 characters.
 * ----------------------------------------------------------------------------
 * Added: this "Sample Changelog":
 * Added: sample source code documentation
 * Added: MAX_SECRET constant. (Could have used "#define MAX_SECRET 10"
 *                              but prefer "const int MAX_SECRET = 10;"
 *                              even at the cost of a couple of extra bytes)
 * Removed: needless boolean type
 * Removed: final else statement by using "(guess != secret)"
 * Note 1: "do {} while();" is correct in this case, so would a while () {} be!
 * Added: Test for non-integer user input, with appropriate error message.
 * Added: test for integer range, with appropriate error message.
 * Replace: return 0; for return EXIT_SUCCESS;
 */
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int main(void) {
  const int MAX_SECRET = 10;
  int secret;
  int guess = 0;
  int guess_count = 0;
  int input_trail_count;
  /* Seed - init the pseudo-random generator */
  srand(time(NULL));
  secret = rand() % MAX_SECRET + 1;
  do {
    printf("Guess my number between 1 and %d inclusive: ", MAX_SECRET); 
    scanf("%d", &guess);
    /* Handle possible user string input by consuming
       and counting any remaining input characters */
    input_trail_count = 0;
    while ('\n' !=  getchar()) { ++input_trail_count; };
    if (0 == input_trail_count) {  /* no trailing chars after integer */
      if (guess > 0 && guess <= MAX_SECRET) { /* integer within range */
        if (guess < secret) {
          printf("Your guess [%d] is too low. Try again.\n", guess);
        } else if (guess > secret) { 
          printf("Your guess [%d] is too high. Try again.\n", guess);
        }
        ++guess_count;
      } else { /* integer out of range */
        printf("Error: integer value [%d] is out of range (1-%d) !\n",
               guess, MAX_SECRET);
      }
    } else { /* detected non-integer input: string */
      printf("Error: detected non-integer input, only integers allowed!\n");
    }
  } while (guess != secret);
  printf("You guessed my number [%d] in [%d] tries!\n", secret, guess_count);
  return EXIT_SUCCESS;
}
/* eof */
  • Added: this "Sample Changelog":
  • Added: sample source code documentation
  • Added: MAX_SECRET constant. (Could have used "#define MAX_SECRET 10" but prefer "const int MAX_SECRET = 10;" even at the cost of a couple of extra bytes)
  • Removed: needless boolean type
  • Removed: final else statement by using "(guess != secret)"
  • Note 1: "do {} while();" is correct in this case, so would a while () {} be!
  • Added: Test for non-integer user input, with appropriate error message.
  • Added: test for integer range, with appropriate error message.
  • Replace: return 0; for return EXIT_SUCCESS;
\$\endgroup\$
  • 5
    \$\begingroup\$ We really appreciate that you chimed in with your first answer! I would recommend however that you not just say what you changed, but explain why you would change it. If you add this in I think you would have a really good answer to this question! \$\endgroup\$ – Phrancis Aug 29 '15 at 0:35

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