Skip to main content
Code Review

Return to Answer

added 850 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Very good.

You can simplify your loops a bit in a couple of places.


You have not really implemented error handling! What happens if I type "Bob" when the code is here:

std::cin >> guess;

That will set the std::cin into an error state and you will never exit. When you are epxecting things other than a string you should always check that the read worked.

if (std::cin >> guess) {
 // User has input a valid number.
 // Add code to handle good user input
 guessNumber(guess, target);
}
else {
 // User has input bad input (not an integer)
 // The stream is in a bad state. Any subsequent
 // attempt to read will no fail until you reset
 // the stream
 std::cout << "Bad Input. Ignoring text to end of line\n";
 ignoreChar();
}

You don't implement this part of the code ocrrectly:

At the end of the game, the user should be asked if they want to play again. If the user doesn’t enter ‘y’ or ‘n’, ask them again.

If they enter 'k' it behaves like you entered an 'n'. The definition says you should ask them again for the input.

Very good.

You can simplify your loops a bit in a couple of places.


You have not really implemented error handling! What happens if I type "Bob" when the code is here:

std::cin >> guess;

That will set the std::cin into an error state and you will never exit. When you are epxecting things other than a string you should always check that the read worked.

if (std::cin >> guess) {
 // User has input a valid number.
 // Add code to handle good user input
 guessNumber(guess, target);
}
else {
 // User has input bad input (not an integer)
 // The stream is in a bad state. Any subsequent
 // attempt to read will no fail until you reset
 // the stream
 std::cout << "Bad Input. Ignoring text to end of line\n";
 ignoreChar();
}

You don't implement this part of the code ocrrectly:

At the end of the game, the user should be asked if they want to play again. If the user doesn’t enter ‘y’ or ‘n’, ask them again.

If they enter 'k' it behaves like you entered an 'n'. The definition says you should ask them again for the input.

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Overview

Codereview

Assuming nothing important in here that needs reviewing?

#include "Random.h"

OK.

void ignoreChar() {
 std::cin.clear();
 std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}

The object of type std::mt19937_64 is quite expensive to build. You don't need to re-build it every time you want a random number.

int randNum() {
 std::mt19937_64 rd{std::random_device{}()};
 std::uniform_int_distribution num{1, 100};
 return num(rd);
}

Here I would make rd and num static members of the funciton. This means they will be created once and re-used every time the function is entered.

int randNum()
{
 static std::mt19937_64 rd{std::random_device{}()};
 static std::uniform_int_distribution num{1, 100};
 return num(rd);
}

A switch is good when you have lots of choices. Personally it seems overkill for two.

bool tryAgain(char guess) {
 switch (guess) {
 case 'y':
 return true;
 case 'n':
 return false; // Add a return statement for the "n" case
 default:
 return false;
 }
}

Personally if I was using the switch. I would treat 'y' and 'Y' the same way.

Also you can't tell the difference between an 'n' and an illegal value. I think that would be important especially in a situation where you are adding code to handle error situations.


You print out you win.

void guessNumber(int userGuess, int actualTarget) {
 if (userGuess > actualTarget) {
 std::cout << "Your guess is too high.";
 } else if (userGuess < actualTarget) {
 std::cout << "Your guess is too low.";
 } else if (userGuess == actualTarget) {
 std::cout << "Correct! You win!";
 }
}

But don't you want to keep track of the fact that you won or that you are continuing? Seems like a grat function to return true from when the player wins a game.

Looking at the next function loopFunction() it seems like you are doing the same test all over again. You could simply use the result of this function to break out of the loop.

Also I don't see a test for loosing. You only have 7 chances you should probably say you loose of you run out of chances.


The while() loop here looks like it could be simplified into a for() loop.

void loopFunction(int input, int target, int guess) {
 while (input <= 7) {
 std::cout << "\nGuess #" << input << ": ";
 std::cin >> guess;
 guessNumber(guess, target);
 ++input;
 if (guess == target) {
 break;
 }
 }
}

Looks. good.
But one thing is that if you do things multiple times it can be make you life simpler if you put that in a function or variable.

Here you print out the string "Let's play a game. I'm thinking of a number between 1 and 100. You have 7 tries so guess what it is." multiple times. Put that text into a variable so it can be re-used. That way if in the future you need to change the text you only need to change the text in one place.

I also thing you can simplify the loop in main. you repeat the same code before the loop and inside the loop (when the user selects 'y'). Could you not simply do that at the beignning of the loop and break out if they enter 'n' at the end of the loop.

int main() {
 int target = randNum();
 char typeNow{};
 int guess{};
 int input{1};
 std::cout << "Let's play a game. I'm thinking of a number between 1 and 100. You have 7 tries so guess what it is.\n";
 loopFunction(input, target, guess);
 do {
 if (guess != target) {
 std::cout << "\nSorry, you lose. The correct number was " << target;
 }
 std::cout << "\nWould you like to play again (y/n)? ";
 std::cin >> typeNow;
 ignoreChar();
 if (typeNow == 'y') {
 input = 1;
 target = randNum();
 guess = 0;
 std::cout << "\nLet's play a game. I'm thinking of a number between 1 and 100. You have 7 tries so guess what it is.\n";
 loopFunction(input, target, guess);
 }
 } while (typeNow == 'y');
 std::cout << "\nThank you for playing.";
 return 0;
}
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /