I am very new to C++ and I've tried to write a simple number guessing game. I know it is a basic task but I'd really appreciate any feedback that could help me improve my writing. Thanks!
#include <iostream>
#include<cstdlib>
int main(){
//This is a number guessing game.
std::cout << "Welcome to the number guessing game!" <<std::endl;
int num; //is the random number.
int g; //guess
int count; //attempt count(max 5)
//generate random number between 1-100
srand((unsigned) time(NULL));
num = 1+(rand()%100);
//ask for player's guess
while (num != g){
std::cout << "Enter your guess (1-100):" <<std::endl;
std::cin >> g;
if (g>num){
std::cout << "Too high! Try again." <<std::endl;
}
if(g<num){
std::cout << "Too low! Try again." <<std::endl;
}
count++;
}
std::cout << "Correct! You found the correct number in " << count <<" tries." <<std::endl;
return 0;
}
3 Answers 3
A couple of things to think about:
- Don’t use
std::endl
, just output\n
instead. See here. srand
andrand
are also in thestd::
namespace, don’t forget to add that. But these functions are outdated, the random number generator used is really bad (though that doesn’t matter in this case). Get used to using the more modern facilities.- Try to use consistent spacing. Using a good IDE that formats your code for you helps with this. It makes the code easier to read and gives a less disorganized impression. For some people in this profession, the dissonance of inconsistent spacing can be highly jarring.
There are two bugs in your code: you use g
and count
before you assign anything to them (they’re uninitialized). You could initialize g
with something outside the range of num
(e.g. zero). count
must be initialized to zero.
Always enable all warnings in your compiler, and pay attention to them. This bug would have been caught by the compiler.
Now this,
int g; //guess
is silly. Why not
int guess;
If you use proper variable names you’ll need fewer comments. Also it makes the rest of the code easier to read, as you won’t have to refer to the earlier comment. This becomes increasingly important as the size and complexity of the program grows.
In C++ you don’t need to declare variables at the top of a scope. It is always best to declare them where you can immediately initialize them. So instead of
int num; //is the random number.
// ...
num = 1+(rand()%100);
do
// ...
int num = 1+(rand()%100);
At the end of main()
, return 0
is implied. You don’t need to return explicitly.
-
\$\begingroup\$ Thanks a lot, I have fixed bugs and spacing, and replaced the number generator with a mersenne twister. But should not main function return an integer as we defined the return type int main() ? \$\endgroup\$Gizem– Gizem2024年01月09日 08:02:46 +00:00Commented Jan 9, 2024 at 8:02
-
1\$\begingroup\$ @Gizem that's a matter of opinion. Strictly speaking it's not needed because main is a special function. Personally I like to write it for consistency. You can also use
return EXIT_SUCCESS;
if you want to make the intentions clearer (it will still return 0). But that is a matter of opinion and either way is fine. \$\endgroup\$infinitezero– infinitezero2024年01月09日 08:54:25 +00:00Commented Jan 9, 2024 at 8:54
I see that you have:
//attempt count(max 5)
But the code never checks if the count is more than 5? And there also does not seem to be any "failure" message when more than 5 tries have been performed.
One can just keep guessing forever. The code and comments are in conflict, the intention is unclear.
Also try to not use 'magic numbers'. See https://stackoverflow.com/questions/47882/what-are-magic-numbers-and-why-do-some-consider-them-bad
Defining constants like
const unsigned int MAX_ATTEMPT_COUNT = 5;
const int RANDOM_GUESS_MIN = 1;
const unsigned int RANDOM_GUESS_RANGE = 100;
const int RANDOM_GUESS_MAX = RANDOM_GUESS_MIN + RANDOM_GUESS_RANGE;
// (...)
int num = RANDOM_GUESS_MIN+(rand()%RANDOM_GUESS_RANGE);
and incorporating these into your code correctly, will improve readability, maintainability and help with conveying your intentions.
The other answer pointed it out, as for the random number generation, consider using modern C++ methods, instead of relying on C methods (rand
, srand
, time
).
This code is problematic:
std::cin >> g; if (g>num){
If stream reading failed, std::cin
will be in an error state, which we need to check before we attempt to use the value of g
:
std::cin >> g;
if (!std::cin) {
std::cerr << "Input failed\n";
return EXIT_FAILURE;
}
if (g>num){
Yes, user input is hard!
return 0;
Since we've included <cstdlib>
we could use EXIT_SUCCESS
as a clearer version of the numeric value. But, since this is main()
, we can omit the return
- reaching the end of main()
is defined to be the same as finishing successfully.