0

I am trying to make a number guessing game in C in which the computer picks a number and you have to guess which number it is. It states too high if the number you picked is greater than the number to guess and vice versa. However, in my implementation, no matter what number you guess, it is incorrect. I would appreciate if you could tell me what is wrong with the code below.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int main(void){
   srand(time(NULL));
   char instring[1];
   int inint;
   int guess;
   guess=rand();
   while (guess!=inint){
      printf("Guess Number\r\n");
      gets(instring);
      inint=atoi(instring);
      if(inint>guess){
         puts("too high");
      }else if(guess>inint){
         puts("too low");
      }else{
         puts("right");
      }
   }
   return 0;
}
Red
  • 1
  • What do you mean by `it is incorrect.` ? `inint` is not initialized 1st time. Also why `instring` size is only `1` byte. If you entered `min 1 char` also you need `2` byte, plus one for terminating `\0` char. – Achal Apr 16 '18 at 04:44
  • There's probably a memory overflow in `instring` each time you get it. And it surely changes `guess` value by overwritting its bytes in memory, You can't guess it because it changes every loop! – Joël Hecht Apr 16 '18 at 05:21
  • regarding: `gets(instring);` the function: `gets()` has been depreciated for years and is completely removed in the latest C standard. Strongly suggest using: `fgets()` which has the syntax: `char *fgets(char *s, int size, FILE *stream);` – user3629249 Apr 16 '18 at 13:40
  • regarding: `gets(instring); inint=atoi(instring);` a integer string can be (upto) ~17 bytes long, BUT `instring` is only one byte long. So, in all cases, the array: `instring[]` is being overrun. The result is undefined behavior. – user3629249 Apr 16 '18 at 13:46
  • There are a lot of issues with your code. :'( you need to fix them. – Ahmed Masud Mar 29 '19 at 16:55

4 Answers4

1

char instring[1]; A C string needs space for some characters + one extra space for a terminating 0. Your string has a length of 1 so it can only fit the terminating 0. Try increasing it's size to say 32.

Also inint is never initialised before you use it (which is bad) - by some stroke of luck inint may == guess before the user even guesses.

Also see Why is the gets function so dangerous that it should not be used?

You could do:

char buffer[32]
fgets(buffer, sizeof(buffer), stdin);

As a safer way with minimal change or you could look at scanf(). Both are safer than gets

John3136
  • 28,809
  • 4
  • 51
  • 69
  • You are right on the fact of increasing the length of instring to 32 which is more that sufficient for this purpose. What actually was happening was that gets(instring) was writing in instring and when it ran out of space in instring, it started to write in guess. This means that the value of guess changes on every iteration of the loop. This means that the user had to guess a number that keeps on changing which is impossible. Also how do I prevent the user from typing a string which has a length greater than 32? – Red Apr 17 '18 at 00:25
  • @Red. Yep. That is a "standard buffer overrun" it will wipe something out and give undefined behavior. You can use `fgets` with `stdin` or look at `scanf()` - see edit – John3136 Apr 17 '18 at 00:29
  • Thanks but this problem goes a bit deeper than that. Whether I use fgets or scanf, the user can type a number too big and cause inint to overflow and become negative. My code will then print "too low" even though the number that the user typed is greater than the number to guess. How do I go on about fixing this new found issue? – Red Apr 17 '18 at 01:34
  • @Red Sure you aren't overthinking it? Why not read the input as a string and then `if (strlen(instring) > 4) printf("Maximum number is 9999\n");` – John3136 Apr 17 '18 at 01:52
0

There are lot of unclear things in your code, first make it clear.Like inint is not initialized 1st time. Also why instring size is only 1 byte. If you entered min 1 char also you need 2 byte, plus one for terminating \0 char. So increase the size of instring.

int main(void){
        srand(time(NULL));
        char instring[32];/* increase the size */
        int inint = 0;/* u forget to initialize */
        int guess=rand();
        while (guess!=inint){ /* when this condition fails ? */
                printf("Guess Number\r\n");
                //gets(instring);/* don't use gets(), instead use fgets() */
                fgets(instring,sizeof(instring),stdin);
                inint = atoi(instring);
                if(inint > guess){
                        puts("too high");
                }else if(guess>inint){
                        puts("too low");
                }else{
                        puts("right");
                }
        }
        return 0;
}

Also read man 2 rand() & check what it returns. Finally use fgets() instead of gets().

Achal
  • 11,821
  • 2
  • 15
  • 37
  • I think what you are doing is right but guess may initialized to 0 in your implementation and the loop will end before a single iteration. This means that the user never got to guess the number. Another thing to note is that if the user types too large of a number, inint will overflow and become negative. Your code will then print "too low" even though the number that the user typed is greater than what was supposed to be guessed. How would I go on about fixing these issues? – Red Apr 17 '18 at 01:17
  • _the loop will end before a single iteration ?_ yes possible bcz `rand()` may return `0` also. _if the user types too large of a number, inint will overflow and become negative_ ? yes that too possible bcz `rand()` will generate random num upto one limit & if user enters too big number means overflow occurs & values gets truncated, one thing you can do take `instring` as unsigned type. – Achal Apr 17 '18 at 01:28
  • Its all upto your implementation, you need take these things into consideration and write the code. – Achal Apr 17 '18 at 01:29
0

I implemented this using using scanf; I didn't like the having to allocate a use a char array to hold the string, and then having to use atoi to convert to an integer.

Notice that when I'm initializing rand(), the value that comes after the parentheses is the maximum (minus one). I didn't want zero, so I added one to the number so that our player will be guessing between 1 and 10.

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


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

    int inint = 0;
    int guess = rand() % 10 + 1;

    while (guess != inint) {
        scanf(" %d", &inint);

        if(inint > guess){
            puts("too high");
        } else if (guess>inint){
            puts("too low");
        } else{
            puts("right");
        }
    }

    return 0;
}

As always, let me know if you have any questions.


There was a question about the new line in the input buffer again today with regards to scanf and the space before the format specifier, so I'll just leave this here.

0

Your correct guessing is inside of while (guess != init). That's the reason you will have always an incorrect guessing.