Console guess game

Question

This is the console program that I've written in C++. It's a game where you have to guess a randomly picked number within the range 0-200. I just took a look at syntax and how to get started with C++ and VS and I was able to write this game almost immediately, but tweaking and improving it took me some time. I have a lot of experience with PAWN.

Anyway, does my code look alright? Please let me know if there are any bad practices that you notice or anything wrong.

#include <iostream> // cin(), cout(), printf()
#include <time.h> // time()
#include <windows.h> // Sleep()
/* Variables */
int number;
int tries;
int max_tries;
/* Functions */
void game_init(int try_count);
void game_process();
void game_decide();
int game_check(char input);
int main()
{
 srand((unsigned int)time(NULL));
 game_init(8);
 game_process();
}
void game_init(int try_count)
{
 number = rand() % 200;
 tries = 1;
 max_tries = try_count;
 printf("You have %d tries to guess a random number we've picked for you. (withing a range 0-200)\n", max_tries);
}
void game_process()
{
 do
 {
 printf("\nTry %d/%d: ", tries, max_tries);
 int input;
 scanf_s("%d", &input);
 if (game_check(input) == 1)
 {
 std::cin.get();
 break;
 }
 if (tries == 9)
 {
 printf("\n\n\nUnfortunately, you weren't able to guess the number.\nIt was %d!", number);
 printf("\n\nI wish you more luck the next time. Wanna try again? Type \"yes\" or \"no\".");
 printf("\nDo you want to try again? ");
 game_decide();
 }
 }
 while (tries <= max_tries);
}
int game_check(char input)
{
 if (input == number)
 {
 printf("\nCongratulations! You've guessed the number, it was %d!", number);
 printf("\n\nDo you want to play again? ");
 game_decide();
 return 1;
 }
 else if (input < number)
 {
 printf("\nThe number %d you've entered is lower than the number we're looking for.", input);
 tries ++;
 std::cin.ignore();
 }
 else if (input > number)
 {
 printf("\nThe number %d you've entered is higher than the number we're looking for.", input);
 tries ++;
 std::cin.ignore();
 }
 return 0;
}
void game_decide()
{
 char decision[3];
 std::cin >> decision;
 if (!strcmp(decision, "yes"))
 {
 std::cout << "\n\n\n";
 game_init(8);
 game_process();
 }
 else if (!strcmp(decision, "no")) 
 {
 printf("Quitting...");
 // Close the console after 1 second
 Sleep(1000);
 exit(0);
 }
 else 
 {
 printf("\n\nI'm not sure I understand what are you saying... Can you repeat that again, just \"yes\" or \"no\", please?");
 printf("\nDo you want to try again? ");
 game_decide();
 }
}

Answer 1

• There's no need for printf() and scanf() in C++. Instead, use:

  • std::cout << "Some text..." << aVariable; for outputting, or
  • std::cin >> aVariable; for inputting

• In C++, use <ctime> instead of <time.h>. The latter is a C library.

• Never use global variables, unless they're constants. Instead, pass number and tries to their relevant functions.

• If max_tries is supposed to be a constant, initialize it below the #includes as such:

  const int max_tries = 8;
  

• Since you're using std::rand(), which returns non-negative numbers, you could change number's type to unsigned int.

• You should move game_init()'s contents to main() instead for clarity and easier program termination (via return X).

• game_check() should return a bool since it's conditional. For bools, 0 corresponds to false and 1 corresponds to true.

  It shouldn't need to display anything since that's not its purpose. Instead, move those outputs to game_process(), remove the std::cin.ignore()s, and return the appropriate bools.

  Remove the return 0; it should now return a value of true or false instead.

  The parameter for its prototype and definition should be an int, which you're already passing into it from game_process().

• In game_decide(): instead of using a char array to hold decision, just make it a single char variable. A "y" or "Y" could mean yes, and an "n" or "N" could mean no. Then, your conditional statements could look like this:

  if (decision == 'y' || decision == 'Y')
  // do something
  else if (decision == 'n' || decision == 'N')
  // do something else
  else
  // try again
  

  You're doing a recursive call, which is absolutely unnecessary for this situation. Instead, use a while or do-while loop to handle this.

  It should return a bool instead (for the same reason as game_check()).

  Clear everything from the "yes" and "no" blocks and instead have them respectively return true and false. Then, back at game_process(), allow the loop to end if game_decide() returns false (meaning that the user no longer wants to play). Thus, the program will fall back to main() and terminate nicely. You would then no longer need #include <windows.h>.

Answer 2

Jamal has many good points. Listen to them. In addition:

I'd argue that this is barely C++. Your code is written as if it was C, only with some C++ function calls instead.

1: Take advantage of C++. Your game_ functions should probably be member functions in a Game class. You can make more use of the C++ standard library. In short, strive towards writing C++.

2: Use std::string instead of char[]. C++ strings are dynamic and can grow, they take care of memory management for you and generally make your life much easier. They can also be turned into a C-style string (i.e. a char[]) by calling mystring.c_str().

3: In C++, we use 0 instead of NULL. In C++11, use nullptr instead.

4: Avoid literal constants. Consider for example your if (tries == 9). Why exactly is this 9? Is that the maximum number of tries? Does it hold some magic meaning? If you declare const int MAX_TRIES = 9; somewhere, if (tries == MAX_TRIES) will become much easier to read. As an added bonus, you only have to change the code one place to change the maximum number of tries. I'm also wondering: Why are you checking triesagainst 9, and not against your max_tries variable?

5: Regarding constants and Jamal's answer: C++ doesn't have a set coding convention. You don't have to write constants in UPPERCASE_UNIX_CASE. However, it's a common convention and I think it's a good one. I still think you should restrict the constant's scope as much as possible. It is okay to have a global or semi-global constant, but there's no reason to do so just because you can. If a constant is only used in a function or other restricted scope, then restrict the constant to that scope.

6: Using a while loop is generally more readable than a do...while loop. Unless you really need to force a first iteration (which you don't in this case), prefer while loops.

7: Use a loop inside game_decide instead of having it call itself. Recursion is in many cases wonderful, but this is not the place nor use for it.

8: On a game design level, consider telling the user if a guess is too high or too low, and remember the upper and lower bounds for him/her.

• Yuushi: I don't agree with ALL_UPPERCASE being a common convention in C++ for constants. Rather, usage is conventionally reserved exclusively for macros.
• Loki Astari: Avoid all uppercase identifiers unless they are macros. The convention is MACRO NAMES are all uppercase. This is because macros have no scope thus your identifiers can potentially be trampled. So to avoid clashes with normal identifiers and those used by the pre-processor the convention is used. What you are seeing is that macros often define constants (in C). With C++ constants are more normally type const and thus don't have all uppercase names.
• Loki Astari: I disagree with using 0 as NULL pointer value. It makes no difference to the compiler (they are the same value). So the only reason to use one over the other is to help poor humans. The use of a name gives valuable context that makes the code easier to read (you know it is a pointer rather than an integer). But nullptr is the correct solution.
• Loki Astari: Can't agree that do..while() is any more/less common than while or even for (well for is probably the most prevalent (but just in my experience others may have totally different experience)). The point: Any is fine. Use the one that makes the most sense in the given context and makes the code easy to understand.
• Lstor: @Yuushi, @LokiAstari: I may very well be affected by the code bases I usually work in, and biased to believe that UNIX_CASE is common for constants. I'll keep in mind that both of you object.

Answer 3

The other two have made all the points I would have made.

I would strongly follow the advice of @Lstor to make the a Game class.

The only addition I have is about reading user input.
When dealing with user input (manual were errors can happen) it is best to deal with line input (users are used to typing something followed by a return). So write your code on the same model. When user input is expected read a line and validate. If its not what you expect print an error and try again.

The issue with operator>> for interactive user input is that it often leaves \n character on the input (or when you least expect it skips passed the \n (or multiple \n) looking for more user input)`.

 std::string line;
 do 
 {
 std::cout << "Do you want to continue 'Y/N'\n";
 std::getline(std::cin, line); // Get users input.
 std::transform(line.begin(), line.end(), ::toupper); // uppercase answer
 }
 while (line != "Y" || line != "N");