3
\$\begingroup\$

After receiving feedback for the grading exercise from this list I proceeded to the next one that looked interesting to me, namely implementing the bracketing search for a computer to guess a number from 1 to 100 in 7 or less guesses. I tried to keep what I was advised in my previous question but a few of these points were not relevant in the second exercise. I'm looking for any feedback, but possibly something about:

  • Variable scope and declaration. I've declared variables in the specific functions that use them. Is that how it should be?
  • This feels "heavy handed". All the switches and loops give me kind of feel of code smell. Is this how c++ programs that loop commands are structured? Should I be abstracting that further?

#include <iostream>
#include <chrono>
#include <random>

static const char QUIT = 'q';
static const char HUMAN_GUESSER = 'h';
static const char COMPUTER_GUESSER = 'c';
static const char CHOSEN_NUMBER_IS_HIGHER = 'h';
static const char CHOSEN_NUMBER_IS_LOWER = 'l';
static const char GUESS_IS_CORRECT = 'c';
static const int DEFAULT_MIN = 1;
static const int DEFAULT_MAX = 100;

int guessNumber(int min, int max) {
    return ((max - min) / 2) + min;
}

bool computerGuesser() {
    char guessInformation;
    int min = DEFAULT_MIN;
    int max = DEFAULT_MAX;
    int guess;
    bool abort = false;
    std::cout
            << "Think of a number which the computer shall guess. (Please don't change the number during the task ;)\n";
    for (int numberOfGuesses = 1; !abort; ++numberOfGuesses) {
        guess = guessNumber(min, max);
        std::cout << "Is the number " << guess << "? Answer with higher ("
                  << CHOSEN_NUMBER_IS_HIGHER << ") if the chosen number is higher than the guess, lower ("
                  << CHOSEN_NUMBER_IS_LOWER << ") if it is lower or correct (" << GUESS_IS_CORRECT
                  << ") if computer got it right.\n";
        if (std::cin >> guessInformation) {
            switch (guessInformation) {
                case CHOSEN_NUMBER_IS_HIGHER:
                    min = guess + 1;
                    break;
                case CHOSEN_NUMBER_IS_LOWER:
                    max = guess - 1;
                    break;
                case GUESS_IS_CORRECT:
                    std::cout << "Yeah! Computer got the correct number in " << numberOfGuesses << " guesses\n";
                    abort = true;
                    break;
                default:
                    std::cout << "that's a wrong option\n";
                    abort = true;
            }
        }
    }
    return abort;
}

int makeRandomNumber(int min, int max) {
    auto seed = std::chrono::system_clock::now().time_since_epoch().count();
    std::mt19937_64 generator(seed);
    std::uniform_int_distribution<int> distribution(min, max);
    return distribution(generator);
}

bool humanGuesser() {
    int userGuess;
    int numberOfGuesses = 0;
    int internalGuess = makeRandomNumber(DEFAULT_MIN, DEFAULT_MAX);
    std::cout << "Enter your guess.\n";
    while (std::cin >> userGuess) {
        ++numberOfGuesses;
        if (userGuess == internalGuess) {
            std::cout << "Correct. It took you " << numberOfGuesses << " guesses.\n";
            break;
        } else {
            std::cout << "Incorrect. The internal guess is " << (userGuess < internalGuess ? "higher." : "lower.")
                      << "\n";
        }
    }
    return true;
}

bool dispatchCommand(char command) {
    switch (command) {
        case COMPUTER_GUESSER:
            return computerGuesser();
        case HUMAN_GUESSER:
            return humanGuesser();
        case QUIT:
        default:
            return false;
    }
}

int main() {
    char command;
    std::cout << "Hello, World! Welcome to the super duper number guesser.\n";
    while (true) {
        std::cout << "Who will be playing, you (h) or the computer (c)? Press (q) to quit.\n";
        if (std::cin >> command && dispatchCommand(command)) {
            continue;
        } else {
            break;
        }
    }
}

cmake_minimum_required(VERSION 3.8)
project(bracketing)

set(CMAKE_CXX_STANDARD 17)

set(SOURCE_FILES main.cpp)
add_executable(bracketing ${SOURCE_FILES})
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

This code is really easy to read. It has good naming and avoids "magic numbers." Your variables all seem properly declared and scopes. I see some things that could be improved, but honestly, it's nothing major. Here are my ideas.

Smaller Functions

In your computerGuesser() function I think you could break out some of the functionality into separate functions to make it less "heavy handed" as you put it. For example, prompting the user could be a function, as could the switch statement that decides what to do next. Something like this:

char promptUser(const int guess)
{
    char guessInformation = '\0';
    do {
        std::cout << "Is the number " << guess << "? Answer with higher ("
                  << CHOSEN_NUMBER_IS_HIGHER << ") if the chosen number is higher than the guess, lower ("
                  << CHOSEN_NUMBER_IS_LOWER << ") if it is lower or correct (" << GUESS_IS_CORRECT
                  << ") if computer got it right.\n";
        std::cin >> guessInformation;
        if ((guessInformation != CHOSEN_NUMBER_IS_HIGHER) &&
            (guessInformation != CHOSEN_NUMBER_IS_LOWER) &&
            (guessInformation != GUESS_IS_CORRECT))
        {
            guessInformation = '\0';
            std::cout << "that's a wrong option. Please try again.\n";
        } 
    } while (guessInformation);
}

Note that I changed the above a little bit. If the user enters an invalid entry, it now asks them to try again and gives them another chance instead of aborting.

Next, the function to decide what to do with the result:

bool handleGuess(const char guessInformation, const int numberOfGuesses,
                 int& min, int& max)
{
    bool abort = false;
    switch (guessInformation) {
        case CHOSEN_NUMBER_IS_HIGHER:
            min = guess + 1;
        break;

        case CHOSEN_NUMBER_IS_LOWER:
            max = guess - 1;
        break;

        case GUESS_IS_CORRECT:
            std::cout << "Yeah! Computer got the correct number in " << numberOfGuesses << " guesses\n";
            abort = true;
         break;
    }

    return abort;
}

Then your computerGuesser() function becomes this:

bool computerGuesser() {
    char guessInformation;
    int min = DEFAULT_MIN;
    int max = DEFAULT_MAX;
    int guess;
    bool abort = false;
    std::cout
            << "Think of a number which the computer shall guess. (Please don't change the number during the task ;)\n";
    for (int numberOfGuesses = 1; !abort; ++numberOfGuesses) {
        guess = guessNumber(min, max);

        guessInformation = promptUser(guess);

        abort = handleGuess(guessInformation, numberOfGuesses);
    }
    return abort;
}

Simpler Logic

I generally dislike infinite loops when there's an obvious end condition. The while loop you have in main() has an end condition, so there's no reason to make it an infinite loop. I'd write it like this:

int main() {
    char command;
    std::cout << "Hello, World! Welcome to the super duper number guesser.\n";
    do {
        std::cout << "Who will be playing, you (h) or the computer (c)? Press (q) to quit.\n";
    } while ((std::cin >> command) && dispatchCommand(command));
}
\$\endgroup\$
1
  • \$\begingroup\$ Good points, I realize I've been misunderstanding the usefulness of the do while loop for a while (no pun intended). Thanks! \$\endgroup\$ Oct 10, 2017 at 21:58

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.