I made this simple game code. Can you please give me some advice on how I can improve it?
#include<stdio.h>
#include<stdlib.h>
#include<conio.h>
#include <time.h>
#include <windows.h>
void main ()
{
printf("\t\tWelcome to our gessing game\n\n\n");
printf("the computer choose a random number between 1-99, try to gess the random number...");
srand(time(NULL));
int gess,i=0,found=0, r = rand()%100;
while (i<10 && found==0)
{
printf("\n My gess is:\t");
scanf("%d",&gess);
if (gess==r)
{
found=1;
i++;
}
else if(gess>r)
{
printf("\n Too big\n");
i++;
}
else if(gess<r)
{
printf("\n Too small\n");
i++;
}
}
if (found==1&&gess==1)
printf("\n Very good the number is %d this is your %dst gess",gess,i);
else if(found==1&&gess==2)
printf("\n very good the number is %d this is your %dnd gess",gess,i);
else if(found==1&&gess==3)
printf("\n very good the number is %d this is your %drd gess",gess,i);
else if(found==1&&gess!=1&&gess!=2&&gess!=3)
printf("\n very good the number is %d this is your %dth gess",gess,i);
else
printf("\n Never mind try again");
getch();
}
3 Answers 3
One variable per line please.
int gess,i=0,found=0, r = rand()%100;
This makes the code hard to read. There is also one corner case (with pointers) were this will not work as expected for beginners. As a result this is usually banned in most companies coding standards.
You are using found
as a boolean variable.
Treat it as such. use TRUE/FALSE.
I would change the while()
into a for(;;)
loop. Then you can remove all the increments.
while (i<10 && found==0)
--
for(int i = 0; i < 10 && !found; ++i)
White space is your friend:
if (found==1&&gess==1) // This is hard to read and will get worse with more conditions.
if (found && (guess == 1)) // much more readable
// ^^^^^ boolean use it as such.
Your "st", "nd", "rd", "th" is not correct.
10-19 => th
x1 => st
x2 => nd
x3 => rd
x[4-9] => th
Also you print basically the same string every time on success. So we not have one print statement and just separate the logic for getting the ending. I would change the logic:
if (!found)
{
printf("\n Never mind try again");
}
else
{
char const* ext = "th";
if ((gess < 10) || (gess >= 20)) // 10->19 already set
{
int end = gess % 10;
if (end == 1) { ext = "st";}
else if (end == 2) { ext = "nd";}
else if (end == 3) { ext = "rd";}
}
printf("\n Very good the number is %d this is your %d%s gess", gess, i, ext);
}
-
\$\begingroup\$ and by the way the game is meant to be 10 guesses otherwise it wont be fun... \$\endgroup\$user2129387– user21293872013年03月09日 18:32:33 +00:00Commented Mar 9, 2013 at 18:32
-
\$\begingroup\$ Even so. I still prefer my bit at the end. It look tidier with one print statement. \$\endgroup\$Loki Astari– Loki Astari2013年03月09日 18:59:31 +00:00Commented Mar 9, 2013 at 18:59
-
\$\begingroup\$ Also, gess->guess. \$\endgroup\$luiscubal– luiscubal2013年03月10日 00:03:12 +00:00Commented Mar 10, 2013 at 0:03
-
\$\begingroup\$ @luiscubal: I don't correct spelling of text output. That;s the job of a manager. It could be another language for all I know. \$\endgroup\$Loki Astari– Loki Astari2013年03月10日 00:14:44 +00:00Commented Mar 10, 2013 at 0:14
-
\$\begingroup\$ shouldn't it be
gess < 10 || gess >= 20
? As it stands, this will never be true. \$\endgroup\$beatgammit– beatgammit2013年03月27日 08:39:38 +00:00Commented Mar 27, 2013 at 8:39
As mentioned in the comments, there's no need for <windows.h>
here. For portability reasons, don't include it unless you're actually using code from it. It has nothing to do with running the program on a Windows system, if that's what you were thinking.
Also, your unformatted output statements (no %
) can use puts()
instead of printf()
. The former also provides a \n
at the end of the output.
#include<stdio.h>
#include<stdlib.h>
#include <time.h>
int main() // Main always return int ...ALWAYS...
{
printf("\t\tWelcome to our gessing game\n\n\n"); // This could be device specific. If i run it on a minimized screen it might look akward
/*
maybe, call a function like take_device_width() then use the value returned to
print the welcome message
*/
printf("the computer choose a random number between 1-99, try to gess the random number...");
/* You could write something like this
#define MIN_NUM 1
#define MAX_NUM 99
printf(""The Computer chooses a random number between %d and %d. \
You have to guess it using minumun number of guesses\n", MIN_NUM, MAX_NUM);
So that when ever you want to change the values for number range you can just
change it once at pre processor macro.
*/
srand(time(NULL)); // srand((unsigned) time(NULL));
/* Dont use bad spellings and always indent code cleanly: i,e int guess, i = 0, found = 0*/
int gess,i=0,found=0, r = rand()%100;
/* INstead of saying that number of guesses is i, you can name it as num_of_guesses. So that next time
when you look at code you will know what num_of_guesses does . Similarly for r.
*/
/* Instead of using magic number 10, you could write it as a macro:
#define MAX_ATTEMPTS 10
*/
while (i<10 && found==0) // indent it here: while(num_of_guesses < MAX_ATTEMPT && found == 0)
{
printf("\n My gess is:\t"); // no need of \t here
scanf("%d",&gess);
/* Indent it. After a block that does something add a comment of what next block does nad write that code */
if (gess==r)
{
found=1; // instead of found we can do this
/* if(guess == r)
{
num_of_guesses++;
printf("You guessed the correct answer in %d guesses", num_of_guesses);
exit(0);
}
will avoid checking for while(condition) even after guessing correct answer
*/
i++;
}
else if(gess>r)
{
printf("\n Too big\n");
i++;
}
else // else is enough here
{
printf("\n Too small\n");
i++;
}
/* most of the newline char are unneccesary */
}
/* Indent here */
/* This looks daunting. What are you trying to do here. Why so many conditions */
if (found==1&&gess==1)
printf("\n Very good the number is %d this is your %dst gess",gess,i);
else if(found==1&&gess==2)
printf("\n very good the number is %d this is your %dnd gess",gess,i);
else if(found==1&&gess==3)
printf("\n very good the number is %d this is your %drd gess",gess,i);
else if(found==1&&gess!=1&&gess!=2&&gess!=3)
printf("\n very good the number is %d this is your %dth gess",gess,i);
else
printf("\n Never mind try again");
getchar();
/* it could be
* if(num_of_guesses >= MAX_ATTEMPTS)
{
printf("You couldn't guess it in suggested attempts. Sorry!");
exit(1);
you can even add a yes or no condition to play again
}
*/
return 0;
}// end main
-
2\$\begingroup\$ Your suggestions are hard to read this way, as comments embedded in the code. I suggest to take the comments outside, see how other answerers do it. It will be a lot more readable, and that's the aim, isn't it? Giving answers that are easy to read, just like code should be easy to read, accessible. \$\endgroup\$janos– janos2015年12月12日 18:25:22 +00:00Commented Dec 12, 2015 at 18:25
if
against the left side). Its hard to read code at the best of times try not to make it harder with bad formatting. \$\endgroup\$