Need Review for a Program I just made.

This is a discussion on Need Review for a Program I just made. within the C Programming forums, part of the General Programming Boards category; Hello everyone! This is my first time posting on this forum, and I'd like for some people to review my ...

Need Review for a Program I just made.

Hello everyone! This is my first time posting on this forum, and I'd like for some people to review my code for a small game I made. It's a typical Random Number Guessing Game, and I was wondering how I could improve this code. I am still new to C, although not new to programming. I started with Python.

In C/C++ the operator= is the assignment operator, and operator== is the comparison operator. When you use the assignment operator in your while loop you have an infinite loop, which is equilavent to while(1).

Also the srand() function should be called only once.

And you need to study up on the scanf() format specifiers, the "%s" specifier is for a C-string, not a single character, tryAgain is a single character.

Lastly getch() is a non-standard function that you should avoid. You should really prefer the getchar() function instead. And if you must use getch() you need to #include the proper include file for this non-standard function.

You declared the variable named running, but do not use it other than to assign 1 to it in the condition of the while loop.

You seeded the PRNG by calling srand within the loop, which means it happens on every iteration. Rather, this should be done only once, usually near the start of the main function.

There does not appear to be any good reason for userIn to be declared static. If you want it to retain its value between iterations, declare it before the loop. However, since it obtains its value from the scanf call, it could well be declared non-static within the loop and it will still work.

Instead of writing:

Code:

random = rand();
random = random % 100;

write:

Code:

random = rand() % 100;

Avoid magic numbers. You want to limit the user to 7 tries, to declare a named constant with the value of 7, then use it for both the printf and to control the for loop.

Check the return value of scanf to determine if the user entered invalid input. Your if-else chain checks if userIn is greater than, less than or equal to the random number... thus the final else clause will never be reached.

The %s format specification for scanf is meant for reading a string into an array. If you do not specify the field width, it makes you vulnerable to buffer overflow. In this case, you are trying to use it to read into a char, which is wrong. Use %c to read a char.

The getch function is non-standard. You can run your program from within a separate command prompt window.

In order to implement this properly, a good method might be to have the user choice to play the game in a while loop that surrounds the game (which I would move to its own function). It's always a good idea to break unique behaviors into their own functions, it might not matter much in a small game like this, but it is vital once you get larger code going.

I wrote a little game in the same manner as the one you have (number guessing) to show you what I mean:

In the main function you have the outer loop, which asks if the player wants to play. If the player is on their 2nd game or later, a ternary operator will add "again" to "Do you want to play?". You have a bit of checking the return of scanf, and also putting the player choice variable through a function called tolower() which will make it lowercase (so we can compare it). If input is invalid, I flush the input buffer with nlkill() (getchar !='\n').

Then the guessing_game() function is the body of the actual game, it's in its own function so that I can easily see where it is, and know what behaviors it should display. This function then uses the compare function to determine if the player has won.

On a final note, one thing you might want to think about (not really a coding thing), is the conceptual side of design. For instance, in a game like this, if the player uses the most efficient manner of guessing, is there a statistical likelihood they could still lose (would you want them too?). As it is, using a simple binary search, the player is almost certainly going to win.

In order to implement this properly, a good method might be to have the user choice to play the game in a while loop that surrounds the game (which I would move to its own function). It's always a good idea to break unique behaviors into their own functions, it might not matter much in a small game like this, but it is vital once you get larger code going.

One rather significant problem with the code Alpo quoted is that tryAgain is a single char, and %s can potentially read to multiple chars. If the user enters almost anything other than whitespace this code exhibits undefined behaviour - which is a problem as this code is being used to read a yes/no type responses.

If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.