5
\$\begingroup\$

This is my code, which chooses a random number from 0 to 10 for the user to guess.

//guess the number game
//my code
#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>

using namespace std;

int main()
{
  unsigned int secretNumber; 
  int guess;
  int maxNumber = 10;
  int maxTries = 4;
  int numTries = 1;


  srand(static_cast<unsigned int>(time(0)));
  secretNumber = (rand() % 10)+ 1;
  cout << "GUESS A NUMBER FROM 0 TO 10!!\n";
    do {
      cout << "\nGuess: \n";
      cin >> guess;

      if (guess < secretNumber)
      {
          cout << "too low:(:(!!      ";
          numTries++;
          cout << "Guesses Left: " << maxTries - numTries;
      } ***//Would it be better to add a bool in the condition?***

      else if (guess > secretNumber && guess <= maxNumber)
      {
          cout << "Too high:D:D!!      ";
          numTries++;
          cout << "Guesses Left: " << maxTries - numTries;
      }

      else if (guess > maxNumber)
      {
          cout << "Do you know how to count to 10?\n";
          cout << "Only from 0 TO 10!!  ";
          numTries++;
          cout << "Guesses Left: " << maxTries - numTries;
      }

      else {
          cout << "WOW! you GUESSED IT?! AMAZING!!!!";
          cout << "You're right! the number is " << guess; 
          cout << "\nYou got it right in " << numTries << " guesses!!!";
      }

      if (numTries == maxTries)
      {
          cout << "\n\nYou LOOSE :( LOL!";
      }


  } while (guess != secretNumber && maxTries != numTries);

  return 0;
} 

This is the teacher's code, which is simpler and includes a bool variable. Should my previous code be simpler, just as this one?

int main()
{
  int secretNumber = 7; 
  int guess;
  int numTries = 0;
  int maxTries = 3;
  bool outOfGuesses = false;

  while (secretNumber != guess && !outOfGuesses)
  {
      if (numTries != maxTries)
      {cout << "Guess a Number: ";
      cin >> guess;
      numTries++;}

      else 
      {
          outOfGuesses = true;
      }
  }

  if (outOfGuesses)
  {
      cout << "You loose!";
  }
  else 
  {
      cout << "You win!";
  }
  return 0;
}

Is my code as efficient and simple as the teacher's code? //Is there a simpler way to do what I intended to do in my code?

\$\endgroup\$
  • 8
    \$\begingroup\$ The teacher's code doesn't do as much as yours does, and it also has Undefined Behavior (using "guess" before it is initialized). So there's not really much basis to compare them. \$\endgroup\$ – 1201ProgramAlarm Aug 11 at 3:34
  • 2
    \$\begingroup\$ Welcome to Code Review. The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. \$\endgroup\$ – Zeta Aug 11 at 4:23
  • 2
    \$\begingroup\$ Also, keep in mind that every content you post is licensed under CC-BY-SA. Your teacher's code is most-likely not available under that license. \$\endgroup\$ – Zeta Aug 11 at 4:24
  • 4
    \$\begingroup\$ A small bug in your code about the secret number: rand()%10 returns a number between 0 and 9 (inclusive). Then (rand()%10)+1 would then return a number between 1 and 10 (inclusive). If you want to a random number between 0 and 10, you need to rand()%11 \$\endgroup\$ – G. Ko Aug 11 at 13:56
  • 4
    \$\begingroup\$ @NellaCrystal What Zeta is saying is that you should not post code that you have not personally written. So it is probably best not to post your teachers code. Though your teacher should probably ask for his own code review :-) \$\endgroup\$ – Martin York Aug 11 at 16:33
15
\$\begingroup\$

Preface

I'm going to ignore your teacher's code, at least for the moment, and just review yours.

Based on what you've said, I'm guessing you're still pretty close to the beginning of the learning curve. Based on that, I'm going to go into a little more detail that normal about how to move in a direction I think you'll find beneficial, rather than just talking about where you might like to end up.

Approach

First of all, you currently have all your code in main. It can be extremely helpful to define small, self-contained functions to carry out the overall task, instead of having it all inline in a single function like this.

To do this, I'd start with a really basic outline of what the program is supposed to do:

  1. generate a random number
  2. While they haven't used up their guesses or gotten the right answer:
    • get a guess at the random number from the user
    • check whether their guess is high, low, or correct
      • print out the result

Then I'd consider which of those is easily turned into a separate, self-contained piece of code (hint: most of them).

Then I'd write code in main that worked at pretty much that level, and have it delegate the details to other code. A first stab at it might look something like this:

int main() { 
    int secretNumber = generate_random();
    int maxTries = 3;

    for (int guessCount = 0; guessCount < maxTries; guessCount++) {
        int guess = get_guess();
        if (check_guess(guess, secretNumber))
            break;
    }
}

Initially, I wouldn't worry a lot about getting every detail precisely correct. Just try to get something that fits reasonably well with the outline you write in English.

From there, you have a couple of choices. One is to start by writing "mock" versions of most of those. They need to do roughly the right sort of thing, but don't put any effort into really making them do the job correctly. For example, we can write a "mock" "generate a random number" as something like:

int generate_random() { 
    return 7;
}

That obviously won't do in the long term, but it's enough that we can use it to write and test the rest of the code. Then do roughly the same with the other functions:

bool check_guess(int guess, int secretNumber) {
    // Todo: add code to print out current result
    return guess == secretNumber;
}

int get_guess() { 
    static int guess;

    return guess++;
}

Now we have enough that we can test the basic flow of the program. For example, we can check that when we run it, it doesn't get stuck in a loop; it runs to termination. Once we've established that, we can add enough more to print out each guess, and whether it was right or wrong, and see that as-is, it guesses values for 0 through 10, then quits because it used up the allotted number of guesses. If so, great. If not, we figure out why not and fix that.

Then we change the random number to (say) 5, so it should guess correctly before it runs out of guesses. Then we run that to be sure it does what it should (like, print out the message that you got the right answer, and then quit asking for more guesses once 5 is guessed).

Once we're done verifying that the basic flow of the code in main works correctly, we can expand out those subordinate functions to do their jobs correctly, so generate_random() actually generates a random number, get_guess() really asks the user for input, and do on.

The important point here so to break the large, somewhat complex task down into a number of smaller tasks, each of which is quite simple. This way, it's much easier to define and understand what each piece needs to do, and test the code so we can be sure that it does what it's really supposed to.

Other Points

I think it's worth pointing out that I'm positively impressed with a number of things about your code. You've chosen good, clear variable names, and structured the code so it's really quite clean and understandable. It's longer than your teacher's, but length is rarely a good measure of much of anything, and its length doesn't seem (to me) particularly excessive for what it does. A fair amount of the extra length is simply because your teacher's code is closer to what I've recommended as the first step on the way toward better code--for example, it chooses the same "random" number every time it's run. That's simple and easy to test with, but yours is clearly a more complete program in this regard.

As far as the specific question of whether to use a Boolean variable: it doesn't strike me as necessary in this case, but it's also fairly harmless. I can see writing the code either with or without it, and doubt that either is necessarily much better than the other.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ xkcd.com/221 \$\endgroup\$ – pm100 Aug 11 at 14:37
  • \$\begingroup\$ To me it's a bit confusing to have guessCount start at 0. What do you think about for (int guessCount = 1; guessCount <= maxGuesses; guessCount++) { ... \$\endgroup\$ – Toivo Säwén Aug 12 at 9:07
  • 1
    \$\begingroup\$ @ToivoSäwén Given that as of now the variable is not used in the loop body, the initial value is not actually important, with a 0-start loop being more natural for C code. Ultimately though, it depends on what exactly guessCount means to you. It could be 'number of guesses at the start of loop body' or 'number of guesses reached in this iteration'. In this kind of situation I'd just leave a comment with the intended interpretation so that future me knows what's going on quicker. \$\endgroup\$ – Mr Redstoner Aug 12 at 15:24
  • 1
    \$\begingroup\$ @ToivoSäwén: In programming, it's common enough for a loop of N iterations to go from 0 through N-1 that it's probably best to just get used to that being how things are done. \$\endgroup\$ – Jerry Coffin Aug 12 at 15:37
  • 1
    \$\begingroup\$ @ToivoSäwén fair enough, that's exactly why I would have had a comment with the intended interpretation of the name. And I too would have made just an i indexed loop for simple code like this. \$\endgroup\$ – Mr Redstoner Aug 13 at 13:22
5
\$\begingroup\$

Read some more comments at the same question at:

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

One small thing I haven't noticed the others mention. You check and handle cases

guess < secret
secret < guess <= max
guess > max

Yet

min > guess

seems oddly missing. Your guess is an int, so nothing is preventing me from putting in a negative number. And because your else doesn't check its assumption (secret==guess) I will instantly get the message about winning, yet the loop will continue, because it does its check separately.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.