Guess My Number - computer guesses the user's number

Question

I'm learning C++ right now and one of the activity/tutorial things was to re-write the "High/Low Guess My Number" game - with a twist. I had to re-write it so that the computer had to guess the number.

Here's the exercise prompt:

Write a new version of the Guess My Number program in which the player and computer switch roles. That is, the player picks a number and the computer must guess what it is

And, here's what I wrote:

// Guess My Number - Computer vs Player
#include <iostream>
#include <cstdlib>
#include <ctime>
using namespace std;
int main()
{
 srand(static_cast<unsigned int>(time(0)));
 int tries = 0;
 // short stuff; // Waiting for solution
 bool error = false;
 enum statusTypes {NADA, HIGH, LOW, CORRECT};
 int status = NADA;
 int min = 0;
 int max = 101;
 int guess = rand() % 100 + 1;
 //int previousGuess; // Only for duplicate guess checking
 // ^ Not used currently, 1.1 release will fix duplicate PC guesses
 int toGuess;
 cout << "01010111 01100101 01101100 01100011 01101111 01101101 01100101 00100001\n";
 cout << "Welcome to Guess My Number\n";
 cout << "Computer vs Player Edition\n\n";
 cout << "Please enter your number (between 1 & 100): ";
 cin >> toGuess;
 cin.get();
 while (status != CORRECT)
 {
 ++tries;
 cout << "Computer's Guess: " << guess << "\n";
 //previousGuess = guess; // Part of Duplicate PC guesses (1.1)
 cout << "Press ENTER to continue..." << endl;
 cin.get();
 /*cout << "\n1st Debug Statements:\n" << endl;
 cout << "Here's the current values: \n";
 cout << "Status: " << status << endl;
 cout << "toGuess: " << toGuess << endl;
 cout << "guess: " << guess << endl;
 cout << "tries: " << tries << endl;
 // ^ Debugging Statements*/
 if (guess < toGuess)
 {
 status = LOW;
 //cout << "\nStatus: " << status << "\n"; // Debugging
 }
 else if (guess > toGuess)
 {
 status = HIGH;
 //cout << "\nStatus: " << status << "\n"; // Debugging
 }
 else if (guess == toGuess)
 {
 status = CORRECT;
 //cout << "\nStatus: " << status << "\n"; // Debugging
 }
 else
 {
 error = true;
 cout << "Uh, something went wrong.\n";
 cout << "Here's the current values: \n";
 cout << "Status: " << status << endl;
 cout << "toGuess: " << toGuess << endl;
 cout << "guess: " << guess << endl;
 cout << "tries: " << tries << endl;
 }
 /*cout << "\n2nd Debug Statements\n" << endl; 
 cout << "Here's the current values: \n";
 cout << "Status: " << status << endl;
 cout << "toGuess: " << toGuess << endl;
 cout << "guess: " << guess << endl;
 cout << "tries: " << tries << endl;
 // ^ Debugging Statements*/
 if (status == HIGH)
 {
 max = guess;
 //cout << "\nReached status == HIGH" << endl; // Debugging
 do
 {
 guess = rand() % 100 + 1;
 } while (guess > max || guess < min);
 }
 else if (status == LOW)
 {
 min = guess;
 //cout << "\nReached status == LOW" << endl; // Debugging
 do
 {
 guess = rand() % 100 + 1;
 } while (guess > max || guess < min);
 }
 else if (status == CORRECT)
 {
 cout << "Computer Guessed It!\n";
 cout << "The guess was " << guess << "\n";
 cout << "And it took " << tries << " tries!\n";
 cout << "Thanks for playing!" << endl;
 }
 else
 {
 error = true;
 cout << "Uh, something went wrong.\n";
 cout << "Here's the current values: \n";
 cout << "Status: " << status << endl;
 cout << "toGuess: " << toGuess << endl;
 cout << "guess: " << guess << endl;
 cout << "tries: " << tries << endl;
 }
 /*cout << "\n3rd Debug Statements:\n" << endl; 
 cout << "Here's the current values: \n";
 cout << "Status: " << status << endl;
 cout << "toGuess: " << toGuess << endl;
 cout << "guess: " << guess << endl;
 cout << "tries: " << tries << endl;
 // ^ Debugging Statements */
 }
 return 0;
}

Obviously, this isn't the most crucial of projects. I was just curious how my coding skills are!

• njzk2: I would tend to not have the user input their number, but ask the user to indicate LOW, HIGH, CORRECT at each iteration. (so the user knows the computer actually guessed)
• RPiAwesomeness: @njzk2 Not a half-bad idea! Thanks for the input!
• Jon Story: If you want to be optimal, you could also split the remaining sample in size every time: ie if it's "pick a number 0-100" guess 50, then 25 or 75 depending on if you're high/low. That way you guess in at most 6 (or is it 7?) iterations, rather than 100 random guesses.

Answer 1

First of all, I'd like to commend you for calling srand() in main() only. It's quite common for many to call it elsewhere instead, which messes up the randomization.

Overall, this looks pretty clean for a beginner. Indentation and whitespace is consistent and the code flows okay. Nonetheless, there are still some points to address, and I'll just provide some basics.

• Once you learn about creating functions, I highly recommend you incorporating this program and future ones. Having everything in main() makes code harder to read and maintain, but it's not as severe with this short program. I'd consider that your next step in your learning.

  For instance, you can have a function for all the conditional statements with status. This will remove duplication, thus shortening your code and having that functionality in one place.

  Here's what one conditional may look like with such a function call:

  if (status == HIGH)
  {
   max = guess;
   guess = randNumber(guess, min, max);
  }
  

  You'll just need to define this function that will assign to guess.

• Try not to have so much commented-out code, at least on your Code Review submission. It makes it harder to read and review, though I understand that it's for debugging. That's not important for others, so you can leave out this information at that time.

• I assume NADA refers to an unassigned status type. In either case, you can rename it to sound more relevant to its use.

  As for the enum itself, you can just name it Status. It's a type, so it can be capitalized.

• RPiAwesomeness: Thanks for the response! As for the functions, I definitely want to use functions. I've programmed in Python & BASIC prior to learning C++, so I know about functions. However, I hadn't got to that part of the book at the time of writing this code. I definitely agree that functions are the way to go! :) As for the the commented code, yeah. I noticed that after I posted the question... I usually remove those as soon as the code is fixed - guess I forgot :P. And, as for the NADA, yes. That's just a stand-in. I wanted to use NULL, but obviously couldn't, & so went with something similar.

Answer 2

if (guess < toGuess)
{
 status = ...
}
else if (guess > toGuess)
{
 status = ...
}
else if (guess == toGuess)
{
 status = ...
}
else
{
 ...
}
if (status == HIGH)
{
 ...
}
else if (status == LOW)
{
 ...
}
else if (status == CORRECT)
{
 ...
}
else
{
 ...
}

Trust me, neither of these last else statements will ever, ever, ever, ever happen.

You'd much better do this instead:

if (guess < toGuess)
{
 status = ...
}
else if (guess > toGuess)
{
 status = ...
}
else // guess == toGuess
{
 status = ...
}
if (status == HIGH)
{
 ...
}
else if (status == LOW)
{
 ...
}
else
{
 ...
}

Because if it's not more, and not less, it has to be equal.

If you really, really, really, really think that it can happen, throw an Exception

• user59064: Agreed. Nothing wrong with a bit of defensive programming but that else statement triggering is 'sky just fell in'. I wouldn't even bother with an assert(.).
• RPiAwesomeness: @DanAllen assert(.) being? I'm pretty new to C++ :) Simon: That makes a lot of sense. I will change that!
• 200_success: If you're going to write a comment that says // guess == toGuess, you might as well convert it into an assertion, which serves as an emphatic "active" comment.
• Simon Forsberg: @200_success I put it there to show the OP, not for it to be included in the actual code.
• user59064: Here's a reasonable question about assert(.): stackoverflow.com/questions/21084385/… The standard implementation aborts the program when the condition you 'assert' is false. I find that a little extreme particularly inside a unit-test. Where available (in C++) I would replace it with something that throws an exception. But the principle remains the same.

Answer 3

Important Point

Experienced C++ programmers seem to almost universally agree that:

using namespace std;

is overall more harm that it is good. There are too many 'ordinary' names in the vast std namespace for projects of any size to avoid some weird clash that takes people far too long - even post release - to find.

** Less Important Point **

<<std::endl is the recommended way to indicate end of line. You've mixed that with \n.

• Simon Forsberg: Doesn't the use of std::endl flush the output buffer or something? Or have I just dreamt having read that?
• RPiAwesomeness: I actually was wondering about just that. The tutorial book I'm following mixed them as well & I was kinda confused about which to use. I looked around and it seems like using << std::endl flushes the buffer (or something along those lines) and so using \n is a slightly higher-performance way to do this, especially if you aren't too worried about data integrity.
• Jamal: @SimonAndréForsberg: You are correct. It is okay to have it at the end, and not used repeatedly.
• user59064: I have the flushing/non-flushing thing (normally) in the 'true but irrelevant' box. Though I admit it could (in principle) matter in an I/O intensive situation. In my experience by the time you've got there you've moved away from <iostream> to lower level handling. However I accept the points. Upvoted.

Answer 4

I'm going to criticize the computer's algorithm. Right now if it knows the number is in the range [min,max] it's making a random guess in that range. If the guess is close to min, then probably the number is larger than min. So more than half the time, we are left with more than half of the previous range still available.

A better option is something called a binary search. Make the guess be in the middle of the possible range. Then you cut down the possible places by half each step. This is a search algorithm you'll be learning at some point - probably soon.

• RPiAwesomeness: This was only the second chapter in, so I wasn't trying to be super advanced. However, this is definitely something I noticed and was wondering about. Thanks for the input!
• Joel: The ability to use a binary search is one of the big reasons that so much effort goes into sorting lists. If a length N list isn't sorted and we want to find something, on average it takes N/2 steps to find it. If it's sorted and we use a binary search it takes log(N) steps. So if N=1024, it'll take about 512 steps if the list isn't sorted, but 10 if it is.
• Richard: +1 This was my very first thought. But I've stopped looking at formatting and minor variances and learned to look at the algorithm itself. My second thought was to check that the binary search didn't overrun itself. (And then there was no binary chopping.) :)

Answer 5

Correct me if I am wrong:

• Dont trust the user blindly. If you ask for 1-100, dont expect it. Validate the input else you might never recover.

• Just curious about this cout. What's this for?

  cout << "01010111 01100101 01101100 01100011 01101111 01101101 01100101 00100001\n";
  

• How does a fourth condition appear

  if (guess < toGuess)
  else if (guess > toGuess)
  else if (guess == toGuess)
  else // ??? For what
  

• What about having the next guess calculated like this:

  if(status == HIGH)
  {
   max = guess;
  }else if(status == LOW)
  {
   min = guess;
  }
  guess = (rand()% (max-min)) + min;
  

• Don't be pessimistic :) Have the status==CORRECT check before LOW and HIGH.

Btw, wouldn't it make more sense by asking the user to make a guess about a computer's number. It seems quite unnecessary for the poor computer to have the actual number but still end up guessing. Damn humans! :P

• njzk2: -1 for suggesting the use of if blocks without {}
• thepace: That's harsh. It was more of a informal code. Will udpate it.
• Simon Forsberg: As is said in the question, the computer-guesses-human twist is "version 2". Also, it is much less likely that status == CORRECT so it does actually makes sense having that condition last.
• Simon Forsberg: @RPiAwesomeness Computers don't derp, programmers do.
• user59064: I think it's harsh too. The routine {} blocking of if is very a widely adopted practice but not a universal requirement. To me the no-no is placing the conditioned statement on the same line as the condition making it hard to put a break-point on the triggered condition in an interactive debugger and I wouldn't vote that down either. IMHO down vote should be used for answers that are actually invalid rather than stylistically wanting or sound but capable of improvement.

Answer 6

So far, no one commented on your formatting.

 else if (status == LOW)
 {
 min = guess;
 //cout << "\nReached status == LOW" << endl; // Debugging
 do
 {
 guess = rand() % 100 + 1;
 } while (guess > max || guess < min);
 }

I'd write it rather as

else 
 if (status == LOW){
 min = guess;
 //cout << "\nReached status == LOW" << endl; // Debugging
 do{
 guess = rand() % 100 + 1;
 } while (guess > max || guess < min);
 }

Separate lines for '{' are imho a waste of space. And you even waste one for 'do' and one for '{'. You can only see a limited number of lines on your screen. More context can help you to write your code faster and it also simplifies reading the code later as you don't have to scroll too much.