Simple Number guessing game

Question

The question is:

Write a C++ program to play a simple number guessing game against a computer opponent. The rules of the games are as follows:

1. The computer randomly selects a secret number between 0 and 100.
2. The user enters a number between 0 and 100 as their secret number.
3. The computer will then attempt to guess the user's number. This guessed number should be printed to the screen and if it is less than the user's secret number, the program should print, "The guess is too low"; if the guess is greater than the user's secret number, it should print "The guess is too high".
4. The user will then attempt to guess the computer's secret number. This guessed number should be printed to the screen and if it is less than the computer's secret number, it should print "The guess is too low"; if the guess is above the user's secret number, it should print "The guess is too high".
5. Repeat steps 3 and 4 until either the computer or the user correctly guesses the other's secret number.
6. When one of the players guesses the other's number correctly, our program should state if the computer or the user won, and then our program should exit. Write a program that repeats round until the player decides to quit.

Here is the code written by me for above question. I have tried it with different inputs and it gives the correct output:

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

bool playerTurn(int compSecret, bool isWinner)
{
  int guess;
  std::cout<<"Enter your guess = ";
  std::cin>>guess;
  if(guess > compSecret)
  {
    std::cout<<"Your guess is to high\n";
    isWinner = false;
  }
  else if(guess < compSecret)
  {
    std::cout<<"Your guess is too low\n";
    isWinner = false;
  }
  else
  {
    std::cout<<"Congratulations you won";
    isWinner = true;
  }
  return isWinner;
}

void computerTurn(int playerSecret, int compSecret)
{
  bool isWinner;
  int low = 0;
  int high = 100;
  do
  {
    int guess = (low + high)/2;
    std::cout<<"Computer's guess = "<<guess<<"\n";
    if(guess == playerSecret)
    {
      std::cout<<"Computer Won !\n"
               <<"Sorry you lost\n"
               <<"Computer's guess was "<<compSecret<<"\n";
      isWinner = true;
    }
    else
    {
      if(guess > playerSecret)
      {
        std::cout<<"Guess is too high\n";
        high = guess - 1;
      }
      else if(guess < playerSecret)
      {
        std::cout<<"Guess is too low\n";
        low = guess + 1;
      }
      isWinner = playerTurn(compSecret, isWinner);
    }
  } while (isWinner == false);
}



int main()
{
  char ch;
  do
  {
    srand(time(0));
    int playerSecret;
    int compSecret = rand()%100;
    std::cout<<"Enter your secret number between 0 and 100 = ";
    std::cin>>playerSecret;
    computerTurn(playerSecret, compSecret);
    std::cout<<"Enter 'Y' to play again :- ";
    std::cin>>ch;
  } while (ch=='y' || ch=='Y');
}

I am a beginner to C++ and wrote the above code with my best understanding. I haven't touched topics like arrays, classes and pointers etc. As a newbie to C++ what changes can I make to improve the above code ?

Answer 1

#include<cstdlib>
#include<time.h>

I recommend using the C++ headers here:

#include <cstdlib>
#include <ctime>

We'll then call std::time() rather than time().

---

  std::cin>>guess;
  if(guess > compSecret)

Before using guess, we need to determine whether the >> conversion was successful. That's actually harder to get right than it sounds, if we want to retry reading (we need to ignore up to the next newline first, and take care not to retry if we reached the end of input).

---

playerTurn() accepts isWinner as parameter, but just overwrites it. So it can be a local variable:

bool playerTurn(int compSecret)
{
  int guess;
  ⋮

  bool isWinner;
  if(guess > compSecret)
  {
    std::cout<<"Your guess is too high\n";
    isWinner = false;
  }
  else if(guess < compSecret)
  {
    std::cout<<"Your guess is too low\n";
    isWinner = false;
  }
  else
  {
    std::cout<<"Congratulations you won";
    isWinner = true;
  }
  return isWinner;
}

Actually, we don't need the variable at all, if we return immediately when we have a result:

#include <limits>

bool playerTurn(int compSecret)
{
    int guess;
    std::cout << "Enter your guess = " << std::flush;
    std::cin >> guess;
    while (!std::cin) {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        if (std::cin.eof()) {
            return false;
        }
        std::cout << "Enter a valid number: " << std::flush;
        std::cin >> guess;
    }

    if (guess > compSecret) {
        std::cout << "Your guess is too high\n";
        return false;
    }
    if (guess < compSecret) {
        std::cout << "Your guess is too low\n";
        return false;
    }
    std::cout << "Congratulations, you won";
    return true;
}

---

computerTurn() is poorly named, as it plays an entire game, not just the computer's turn.

---

isWinner == false is usually written !isWinner (pronounced "not isWinner").

We don't really need isWinner, if we use break to leave the loop when the computer wins.

---

In main(), we need to use std::srand and std::rand, since that's what <cstdlib> is guaranteed to define. Some compilers may also define the global-namespace versions, but that's not portable to assume.

Answer 2

One thing I don't like about this exercise is that it involves user/computer interaction. Without any prior preparation, it is asking you to invent your own ad-hok somewhat robust input system, and that is not a simple thing. The chore of getting a valid and legal number input is distracting from the real logic of the problem!

IMO, I think such issues should at least be already supplied to you as given code, with a framework or skeleton application, making it clear which code you are responsible for writing.

What we can do though is rely on good top-down decomposition. You define a helper function for getting the input, and how fancy that subtask needs to be developed is kind of vague and and be separated from the main problem.

Next, go over the specifications. I'm concerned with:

number between 0 and 100

Is that an integer (fair assumption) that may include 0 and may include 100 as well? That seems odd. I would expect a human facing round number to be 1 through 100 inclusive, or when using zero-based indexing 0 through 99 inclusive.

Also, the order of events is strange. I would not expect step 1 to be done so far ahead of its need; it should be stuck together with step 4. And why do you need to tell the computer your "secret number" before the computer tries to guess it? It appears that the high/low testing feedback is automatic based on the that, rather than having you answer "too high" or "too low" each time.

Ah, step 1 is in advance because both guesses are taking place at the same time. They take turns guessing each other's number, rather than doing a complete search.

---

    srand(time(0));
    int playerSecret;
    int compSecret = rand()%100;

You should be learning that the right way to choose an integer in a uniform distribution is with the proper utility for that: uniform_int_distribution. The example on the linked page is a "standard 6 sided die" and it's easy to change the 1..6 into 0..100 (or whatever) without having to understand it in more detail.

Look up the docs for rand and it shows:

There are no guarantees as to the quality of the random sequence produced. In the past, some implementations of rand() have had serious shortcomings in the randomness, distribution and period of the sequence produced (in one well-known example, the low-order bit simply alternated between 1 and 0 between calls).

rand() is not recommended for serious random-number generation needs. It is recommended to use C++11's random number generation facilities to replace rand().

Anyway, you're dividing the int that's between 0 and RAND_MAX inclusive by 100, and assuming RAND_MAX is something like the largest signed int rather than a multiple of 100, you're actually producing a result that does include 0 and does include 100 though with a reduced probability.

---

main calls computerTurn, and then computerTurn calls playerTurn. That is really weird and doesn't make logical sense. They should be independent of each other.

As specified, step 5 defines the loop of taking turns guessing, which implies that it will know whether either of the guessing steps was success. But steps 3 and 4 don't cover guessing correctly! They say what do do when the guess is wrong, and nothing about what to do if it's right. Step 6 says to quit when either step 3 or 4 got it right, and then contradicts itself by saying it should prompt to start over.

So, the specs are not very suitable as pseudocode. Don't try to adhere to them in that manner. Instead, treat this as a "user supplied requirements" which we know will not be rigorous in an engineering sense — at least that's a realistic part of a problem! You need to clarify and rephrase in a more formal way and get the client to agree to the improved detailed specifications.

Based on our understanding of what the program ought to do, here is a simple breakdown:

void main()
{
   do {
       play_game();
   } while (should_repeat());
}

Note that your original main repeat logic is not abstracted enough. The test is low-level code testing for specific letters, and the prompting and input is inside the same loop as the playing, separate from the test. A function should break things down to a single deeper level of abstraction, not (sometimes) ravel all the way down to the most primitive details. Here the function call should_repeat contains the variables needed to handle the input, the prompting, and the related logic, all together in one place, and with nothing else about the game mixed into that.

And, play_game can be written with a single pass in mind.

But, it is not really a single function... it is two functions that should be written separately, but they take turns and need to remember what's happening between steps! I'm assuming you're not going to use C++20's co-routines here (a fairly new and advanced feature), but will present it as an object (state plus multiple functions defined inside it) in a very classic manner. So actually change the "function call" above to an object instantiation and a call on it.

void main()
{
   do {
       game_player g;
       g.play();
   } while (should_repeat());
}

And we have a class:

class game_player {
   int user_secret;
   int computer_secret;
   int computer_high, computer_low;  // state used for computer's turn
   bool user_turn();  // returns true if guessed
   bool computer_turn();
public:
   void play();
};

Now, if you are not familiar with classes and object-oriented programming, the point here is that we collect together everything and make the related functions and "global variables" an isolated unit that can be safely a part of a larger program — as thing are in real life. You could just use plain functions and global variables since this is the only thing in this program, but that's not the right thing to learn.

void game_player::play()
{
   computer_secret = generate_random_secret();
   user_secret = prompt_random_secret();
     ⋮
}

Now look at the first two steps, clearly corresponding to steps 1 and 2 of the original problem description.

Neither one does the work here in place! This is decomposing the problem into high-level steps. The details of how to choose a random number, and the details of prompting and inputting (and checking the result and repeating??) are part of those steps, not something that should be exposed here. Continue in that light: just break out the steps into separate functions, but don't explain how to do them here. Here you just state tham and any simple glue logic that goes between them. So...

     ⋮
    do { /* empty */ }
    while (!computer_turn() && !user_turn());
}

We defined each of those functions to return true if the guess was successful, so keep doing them until that happens, which means while neither happens. You should learn well various identities like De Morgan's Laws so this transformation is intuitive and second nature. Note also that && is _short circuiting` so that if the computer wins it will not run the user's turn again before dropping out of the entire loop.

---

Finally, in order to implement the computer_turn function you are writing a binary search. You will need to test it. Does it work when the thing to be guessed is the largest (100) or the smallest (0) legal values? By having separate functions, you make testing possible, as you can bypass the random numbers and other parts and just drive this little part of the logic directly, with a small helper function. That is, don't play the full game, just have the computer guess only, and pre-load the secret without you having to type it each time you re-test. You could even test the guessing of all number in the legal range and make sure that it always figures it out eventually and in the minimum number of tries, too!

---

I hope that's good food for thought, and some additional code to get you started without spoiling the entire result. Keep up the practice!