“Guess the number” game

Question

Any tips on how to improve my code and make it cleaner to read for other programmers? Formatting, spacing, etc.

#include <iostream>
#include <stdlib.h>
#include <time.h>
#include <cstdlib>
#include <windows.h>

using namespace std;

int main()
{
    int userInput;
    int number; 
    int min = 1;
    int max = 10;
    float playerTimesToPlay = 0; 
    int playAgain; 
    float tries = 3; 

    do
    {
        system("CLS");
        cout << "Welcome to my number guessing game!" << endl; 
        cout << "====================================================" << endl; 
        cout << "The computer is generating a number between 1 and 10." << endl; 
        cout << "====================================================" << endl; 
        srand(time(0)); 
        number = rand()% (max - min + 1)+min; 
        playerTimesToPlay = 0; 
        tries = 3; 



        do
        {

            cout << "Please take a guess: ";
            if (!(cin >> userInput))
            {
            cout << endl;
            cout << "Please enter numbers only." << endl;
            cin.clear();
            cin.ignore(10000,'\n');
            }


            while(userInput > 10 || userInput < 0)
            {
                cout << "Pick a number BETWEEN 1 TO 10." << endl; 
                cout << endl; 
                userInput = NULL;
                playerTimesToPlay=0;
            }


            tries--;
            cout << endl;
            cout << "You picked: " << userInput << endl;
            cout << "======================" << endl;
            playerTimesToPlay++;

            if(userInput != number && tries > 0 && userInput <= 10)
            {
                cout << "Sorry! Invalid choice. Please try again. You have " << tries << " guess(es) remaining." << endl;
                cout << endl;
            }

            else if(userInput != number && tries == 0) 
            {
                cout << "Sorry! Invalid choice." << endl;
                cout << "You have " << tries << " guesses remaining. You lose." << endl;
                cout << endl;
                cout << "Play again? " << endl;
                cout << "1. Yes" << endl;
                cout << "2. No" << endl;
                cout << ": " ;
                cin >> playAgain;
            }

            if(userInput == number)
            {
                cout << "Congratulations! You guessed the correct number!" << endl; 
                cout << "Thanks for playing!" << endl;
                Sleep(2500);
            }




        }
        while (playerTimesToPlay < 3 && userInput != number);
    }
    while (playAgain == 1);


    system("Pause(0)");
    return 0;
}

Answer 1

Headers

• You can remove stdlib.h as it is redundant to use both stdlib.h and cstdlib. The cstdlib is the c++ variant.

• The same goes for the time.h header. Use ctime instead.

• The Sleep() functions are virtually useless, I removed them and also the <windows.h> header along with it. Don't confuse your users with pauses such as those, not to mention the portability issues. Keep it simple and let the program run at its own pace on their machines.

Namespace

• Don't make a habit of utilizing using namespace std;. You will run into trouble later on down the road if you choose to build something that another developer will make use of. It is heavily frowned upon as it will potentially cause namespace collisions, especially if included in a header file for say, a library.

  Instead just explicitly call the functions with the scope operator:

  std::cout << "Don't muddy your scope!";
  

Variables

• You have quite a few extra variables that can be removed, and also ones that could be more productive as a different data type.

  1. The minimum and maximum variables should be declared as constants as they're not configurable anywhere in your code. Also, they could probably be a little more descriptive instead of just 'min' and 'max'.

     const int LOWER_NUMBER_BOUND = 1;
     const int UPPER_NUMBER_BOUND = 10;
     

  2. number is also pretty vague and should be renamed to infer its purpose.

     int numberToGuess;
     

  3. It appears you have two variables keeping track of the same data. You decrement tries when the player guesses incorrectly but at the same time you increment playerTimesToPlay. I would advise removing one of these and sticking to one variable that tracks the guess attempts remaining. It would also be a good idea to include a constant variable that holds the max attempts so you can reset the tracking variable after a game ends.

     const int GUESS_ATTEMPTS = 3;
     int guessAttemptsRemaining;
     

  4. userInput can also be named more clearly. Something to solidify the function of the variable.

     int userGuess;
     

Random

• You're currently reseeding the random algorithm every time you play the game. This is unnecessary and it can be moved out of your loop.

• Consider using the newer <random> instead, however the rand() function is simpler and more easily understood for a beginner. Also your distribution isn't critical so I wouldn't worry about this for the moment.

Logic

• Inside of the inner do loop, the while statement can be altered to reflect when there are guesses remaining instead of using the incremented variable that was removed. It's also redundant to check if the user's guess is correct so that can also be removed.

  do{...} while (guessAttemptsRemaining > 0);
  

• You can shrink the input validation and remove some of the output by simply asking them to enter a number between the bounds.

  while (userGuess < 1 || userGuess > 10)
  {
      std::cout << "Please pick a number between 1 and 10: ";
      if (!(std::cin >> userGuess))
      {
          std::cin.clear();
          std::cin.ignore(10000, '\n');
      }
  }
  

• There is a logic error when checking the bounds of the user's input. Instead of checking if the number is less than 0 it should be if it is less than 1 as 0 is invalid input.

  while (userGuess < 1 || userGuess > 10)
  

• You check for the same conditions multiple times. Instead of repeating yourself you can restructure your logic to only check for a condition once. There is quite a bit of this going on so instead of pointing out every specific example I'll just go ahead and refactor it to show you what I mean.

Output

• Instead of std::endl consider using \n instead. It is faster than a function call and also doesn't have the additional overhead of buffer manipulation that is included with std::endl.

Refactored Code

#include <iostream>
#include <ctime>
#include <cstdlib>

const int LOWER_NUMBER_BOUND = 1;
const int UPPER_NUMBER_BOUND = 10;
const int GUESS_ATTEMPTS = 3;

int main(int argc, _TCHAR* argv[])
{
    int userGuess;
    int numberToGuess;
    int guessAttemptsRemaining;
    int playAgain = 0;

    srand(time(0));

    do
    {
        system("CLS");
        std::cout << "Welcome to my number guessing game!\n";
        std::cout << "====================================================\n";
        std::cout << "The computer is generating a number between 1 and 10.\n";
        std::cout << "====================================================\n";

        numberToGuess = rand() % (UPPER_NUMBER_BOUND - LOWER_NUMBER_BOUND + 1) + LOWER_NUMBER_BOUND;
        guessAttemptsRemaining = GUESS_ATTEMPTS;

        do
        {

            userGuess = 0;

            while (userGuess < 1 || userGuess > 10)
            {
                std::cout << "Please pick a number between 1 and 10: ";
                if (!(std::cin >> userGuess))
                {
                    std::cin.clear();
                    std::cin.ignore(10000, '\n');
                }
            }

            --guessAttemptsRemaining;
            std::cout << "You picked: " << userGuess << ".\n";

            if (userGuess != numberToGuess)
            {
                std::cout << "Sorry! You've guessed incorrectly!\n";

                if (guessAttemptsRemaining > 0)
                {
                    std::cout << "Please try again. You have " << guessAttemptsRemaining << " guess(es) remaining.\n";
                }
                else
                {
                    std::cout << "You have no guess remaining. Play Again?\n";
                    std::cout << "1. Yes\n";
                    std::cout << "2. No\n";
                    std::cin >> playAgain;
                }

            }
            else
            {
                std::cout << "Congratulations! You guessed the correct number!\n";
                std::cout << "Thanks for playing!\n";
                break; //Add some way to continue here?
            }

        } while (guessAttemptsRemaining > 0);

    } while (playAgain == 1);

    return 0;
}

Additional

• I didn't cover everything so I may add more in the future. I also didn't go into extreme detail about the topics I went over so take my additions/alterations/etc. with a grain of salt and do your own research!

Answer 2

It's not too bad if this is one of your first programs in C++. The next thing you should do is to move that code into a class, since the class is one of the most important aspects of C++.

rand():

If your compiler is C++11 enabled, you should consider replacing rand() and srand() with the facilities of the new <random> library. rand() is notoriously unreliable and not suitable for most applications. It should of course be enough for your simple demo, but it is a good thing to start learning with the best practices and most up-to-date tools, so take a look a the new standard random library.

Sleep():

The Sleep() function is not standard. It is a Windows service. In C++11, we use std::this_thread::sleep_for(), which is portable and standard. (Note: #include <thread>)

system():

The system() function is another portability nightmare that you should just stay away from. For system("pause"), you should just use std::cin.get() instead. Clearing the console screen gets more complicated though. The standard C++ library doesn't provide any facility for that. So you either have to keep the non-portable system("cls") or use some platform specific library, also non-portable.

using namespace std:

Generally not recommended. It is not very harmful when using namespace std; in a .cpp file. In a header (.h) file, it is totally inadequate. But I would advise against in any case, since the idea of a namespace is that you can have identical names coexisting without clashes. If you do a using namespace you throw that feature away. And after all, typing std::cout, std::string, etc is not that much more typing. Most IDEs will auto-complete for you once you finish the ::.

Includes:

When you #include the C library files, such as time.h and stdlib.h, you should instead use the C++ equivalents:

#include <iostream>
#include <ctime>
#include <cstdlib>
#include <windows.h>

I've also removed the redundant stdlib.h from the above. cstdlib is the C++ equivalent of stdlib.h.

windows.h is, of course, Windows specific, so that makes your code non-portable.