3
\$\begingroup\$

I did the exercise of LearnCpp.com's Chapter 8 summary and quiz and I'm still learning…

Question #3

Implement a game of hi-lo. First, your program should pick a random integer between 1 and 100. The user is given 7 tries to guess the number.

If the user does not guess the correct number, the program should tell them whether they guessed too high or too low. If the user guesses the right number, the program should tell them they won. If they run out of guesses, the program should tell them they lost, and what the correct number is.
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.

For this quiz, assume the user enters a valid number.

#include <iostream>
#include "Random.h"

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

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

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;
    }
}

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!";
    }
}

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;
        }
    }
}

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;
}

Is the code redundant?
What are the changes I should make?

\$\endgroup\$
1
  • \$\begingroup\$ (Don't let "reputed" users chiming in intimidate you: This is your question. If you think a (well intended) change to one of your posts did not improve it, edit it again!) \$\endgroup\$
    – greybeard
    Dec 25, 2023 at 16:30

2 Answers 2

2
\$\begingroup\$

Overview

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.

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;
}
\$\endgroup\$
1
\$\begingroup\$

This is quite good for someone who just started learning C++. It's good to see the code being split into multiple functions; this allows both code reuse and gives things a name. There are some improvements possible though:

Naming things

A major issue I see with your code is the way you name things. Bad names make it harder to understand the code, both for a reviewer, but also for yourself; after half a year you will have forgotten half of why exactly you wrote the things the way you wrote them.

First, it's great that you didn't abbreviate everything, however some things were still shortened unnecessarily, like num and randNum(), especially since you did write guessNumber(). Avoid unnecessary abbreviations (most code editors can autocomplete these days so it often doesn't save much typing), and be consistent.

Furthermore, the names are a bit misleading; guessNumber() doesn't "guess the number" as its name implies, it only verifies if the guessed number is correct, and prints a hint otherwise. Maybe checkGuess() is a better name.

Try to use nouns for variables and verbs for functions. randNum() looks like a noun, instead try to make it a verb, like generateRandomNumber().

loopFunction() is very badly named: sure it is a function and it contains a loop, but in a bigger program lots of functions will be like that. What does it actually do? It looks like it asks the user for guesses, until they guess right or used up all seven guesses. Maybe... processGuesses()?

typeNow is also badly named. Sure, the user has to type something when a question is asked, but what exactly is stored in this variable? It's the answer the user gave, or the choice they made. So answer and choice are better names.

Similarly, input is not storing any input. Instead, it counts the number of the attempt the user is going to make. So perhaps attempt?

Avoid unnecessary code

You call loopFunction() twice, but that seems unnecessary. Why not have just one call right at the start of the do-while loop in main()? The same goes for printing the text "Let's play a game…".

You also have the variable guess and input in main(), but they are not used at all there, except to pass them to loopFunction(). I would remove them there, and instead declare those variables inside loopFunction() instead. loopFunction() then only needs one parameter: the target number.

Input and output

There are some small issues with the way you print and read things. First, I recommend that when printing things, you always end with a newline character, and never use it at the start unless really necessary. Mixing the position of the newline means you have to know exactly where you print what, to avoid either missing newlines or double newlines.

If you use std::cin >> … to read things, you already know that it doesn't necessarily read until the end of the line. You can do tricks like you did in ignoreChar() to skip all input until the next line, but if you want to process the input line by line, I recommend you use std::getline() instead. This reads the whole line (minus the newline character) into a std::string. To convert this into a number, you can use std::stoi().

Note that either way, the user can still type in unexpected characters. If you want to ensure that they only type just one number on a line, you'll have to do a little bit more work. std::stoi() optionally takes a pointer to a variable where it will store the position of the first character after the number it parsed. If it's the NUL character ('\0'), you know that there was nothing else on the line.

Finally, errors can happen both when printing and reading input from the console. Ideally, you check if std::cin and std::cout are good() after I/O operations, and if not exit your program with EXIT_FAILURE.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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