15
\$\begingroup\$

I created a simple guessing game where the player can choose whether the player is guessing the number or the computer.

If the player is guessing the number, then the computer will generate a random number between 1 to 100. Then, the player must guess the computer's number.

First, the player will type their guessed number. If it is too high than the computer's number, then the program will print out that the player's number is too high if it is too low, vice versa.

If it is correct, then the computer will congratulate the player and ask if the player wants to play again or no. If the player wants to play again, the program will restart, but if the player does not want to play again, the program will exit.

If the computer is guessing the number, then the player will think of a number. The computer will print out a number and ask if the player's number is higher or lower. The computer will keep doing this until it finds the number.

I'm looking for feedback on absolutely everything that could make me a better programmer, especially a better C++ programmer, such as:

  • Optimization
  • Bad practice and good practice
  • Code structure
  • Functions and variable naming (to be honest, I'm not really good at naming, lol)
  • Bugs
  • etc

Thank you very much!

I'm using Visual Studio Community 2019 ver 16.7.6

Globals.h

#ifndef GUARD_GLOBALS_H
#define GUARD_GLOBALS_H

static const char COMPUTER_GUESSER = 'c';
static const char PLAYER_GUESSER = 'p';
static const char QUIT = 'q';
static const char ANSWER_IS_YES = 'y';
static const char ANSWER_IS_NO = 'n';
static const int MAX_NUMBER = 100;
static const int MIN_NUMBER = 1;

#endif

BracketingSearch.h

#ifndef GUARD_BRACKETINGSEARCH_H
#define GUARD_BRACKETINGSEARCH_H

int randomNumGenerator(const int max, const int min);
int rangeNumToGuess(const int max, const int min);
int rangeNum(const int max, const int min);

bool startGame();
bool computerOrPlayer(const char userchoice);

bool computerGuesser();
bool playerGuesser();

bool restart();

#endif

BracketingSearch.cpp

#include <iostream>

#include "Globals.h"
#include "BracketingSearch.h"

int randomNumGenerator(const int max, const int min)
{
    return rand() % max + min;
}

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

int rangeNum(const int max, const int min)
{
    return max - min;
}

bool startGame()
{
    char userChoice{};

    std::cout <<
        "Who will be the guesser?\n"
        "C - for computer\n"
        "P - for player\n"
        "Q - for quit\n"
        "Type one of the choice: ";
    std::cin >> userChoice;

    computerOrPlayer(tolower(userChoice));
    restart();

    return true;
}

bool computerOrPlayer(const char userchoice)
{
    if (userchoice == COMPUTER_GUESSER)
    {
        return computerGuesser();
    }
    else if (userchoice == PLAYER_GUESSER)
    {
        return playerGuesser();
    }
    else if (userchoice == QUIT)
    {
        std::cout << "Thank you for playing\n";
    }
}

bool computerGuesser()
{
    char userInput{};
    int maxNum = MAX_NUMBER;
    int minNum = MIN_NUMBER;
    int guessNum{};
    int guessCount{ 1 };
    int range;

    std::cout << "Think of a number between 1 to 100\n";

    while(maxNum != minNum)
    {
        ++guessCount;
        range = rangeNum(maxNum, minNum);

        if (range == 1)
        {
            guessNum = maxNum;
        }
        else
        {
            guessNum = rangeNumToGuess(maxNum, minNum);
        }

        std::cout << "Is your number less than: " << guessNum << "?(y/n): ";
        std::cin >> userInput;

        switch (userInput)
        {
        case ANSWER_IS_YES:
            maxNum = guessNum - 1;
            break;
        case ANSWER_IS_NO:
            minNum = guessNum;
            break;
        default:
            std::cout << "That is a wrong option\n";
            guessCount -= 1;
            break;
        }

        if (maxNum == minNum)
        {
            std::cout << "Your number is: " << maxNum << std::endl;
            std::cout << "It took " << guessCount << " guesses for me to guess" << std::endl;
        }

    }
    return true;
}

bool playerGuesser()
{
    int userGuess{};
    int guessCount{ 1 };
    int number = randomNumGenerator(MAX_NUMBER, MIN_NUMBER);

    std::cout << "Enter your guess number: ";

    while (std::cin >> userGuess)
    {
        ++guessCount;

        if (userGuess > number)
        {
            std::cout << "Too high!\n";
        }
        else if (userGuess < number)
        {
            std::cout << "Too low!\n";
        }
        else if (userGuess == number)
        {
            std::cout << 
                "Your guess is correct!\n"
                "It took you: " << guessCount << " guesses\n";
            break;
        }

        std::cout << "Guess another number: ";
    }
    return true;
}

bool restart()
{
    char userChoice{};
    std::cout << "Play again? (y/n): ";
    std::cin >> userChoice;

    char lowerUserChoice = tolower(userChoice);

    if (lowerUserChoice == ANSWER_IS_YES)
    {
        startGame();
    }
    else if (lowerUserChoice == ANSWER_IS_NO)
    {
        computerOrPlayer(QUIT);
    }
    else
    {
        std::cout << "Please choose the available option\n";
        restart();
    }

    return true;
}

main.cpp

#include "BracketingSearch.h"
#include <cstdlib>
#include <ctime>

int main()
{
    srand((unsigned)time(0));

    startGame();

    return 0;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Good first question! However, you should edit your title so that it states what your code does, you don't have to describe your code in the title. Something like "Number guessing game in C++" sounds like a good idea to me, you can decide :) \$\endgroup\$ – Aryan Parekh Oct 26 at 6:18
  • 1
    \$\begingroup\$ Oh, ok, thanks, Mate! \$\endgroup\$ – Sir Broccolia Oct 26 at 6:19
13
\$\begingroup\$

General observations

To be honest, your code is extremely clear and readable to me. I wouldn't guess that you were a beginner from reading your code. You have eliminated the use of magic numbers, and use global constants instead which is good!


Anonymous namespaces

The keyword static in this context means that it has internal linkage. An anonymous namespace also does the same thing, but they are considered to be somewhat superior to the static keyword in C++.

The link I cited has some great answers.
But mainly,

  • static will only work for functions and objects, an anonymous namespace on the other hand can let you have your own type definitions, classes, structs (almost everything)...
// Globals.h

namespace 
{
    // constants
}

Prefer using constexpr

constexpr in C++

The keyword constexpr was introduced in C++11 and improved in C++14. It means constant expression. Like const, it can be applied to variables: A compiler error is raised when any code attempts to modify the value. Unlike const, constexpr can also be applied to functions and class constructors. constexpr indicates that the value, or return value, is constant and, where possible, is computed at compile time.

Use constexpr when you can, it tells the compiler that it's literally just a constant.
It forces the compiler to calculate the value of something at compile-time. Moreover, you can pass it as a template argument too

namespace 
{
    constexpr char COMPUTER_GUESSER { 'c' };
}

Use an enum

This point can depend according to your style, but I think that an enum is called for here.

I am talking about these variables

COMPUTER_GUESSER = 'c';
PLAYER_GUESSER = 'p';
QUIT = 'q';
ANSWER_IS_YES = 'y';
ANSWER_IS_NO = 'n';

I believe that having an enum here makes sense because you can group these variables as they all are related to the user's choice, this is what it would look like

enum Choice : char 
{
    COMPUTER_GUESSER = 'c',
    PLAYER_GUESSER = 'p',
    QUIT = 'q',
    ANSWER_IS_YES = 'y',
    ANSWER_IS_NO = 'n',
};
if (input == Choice::QUIT) //...

else if (input == Choice::ANSWER_YES) //...

Generating a random int

C++ has std::uniform_int_distribution which is better than C's rand().


Consider inlining smaller functions

int randomNumGenerator(const int max, const int min)
{
    return rand() % max + min;
}

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

int rangeNum(const int max, const int min)
{
    return max - min;
}

Inlining these functions can improve the performance a lot, but you need to place the definition of these functions in the header file, you can specify inline but it's likely that the compiler will inline them itself.

instead of executing the function call CPU instruction to transfer control to the function body, a copy of the function body is executed without generating the call.


Always handle invalid input

std::cout << "Enter your guess number: ";

while (std::cin >> userGuess)
{
    //...
}

Here, std::cin is expecting an integer, if the user accidentally enters something else, std::cin will fail, leading to strange behavior in your program

There are a few ways, this article is worth reading.

A small bug

In your restart() function

bool restart()
{
    char userChoice{};
    std::cout << "Play again? (y/n): ";
    std::cin >> userChoice;

    char lowerUserChoice = tolower(userChoice);

    if (lowerUserChoice == ANSWER_IS_YES)
    {
        startGame();
    }
    else if (lowerUserChoice == ANSWER_IS_NO)
    {
        computerOrPlayer(QUIT);
    }
    else
    {
        std::cout << "Please choose the available option\n";
        restart();
    }

    return true;
}

Since you recursively call restart() on invalid input, you should return the value you get. Otherwise, the function won't return anything

else 
{ 
    std::cout << "Please choose a valid option!\n";
    return restart();
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I've been struggling learning C++ but your compliment really boost my confidence. Honestly, I don't know what "static" does. I just saw someone uses it and then I think, why not, lol. I don't understand about the inline part but I'll definitely learn more about it. Thank you very much, Mate, your feedback will greatly help me become a much better programmer \$\endgroup\$ – Sir Broccolia Oct 26 at 10:12
  • 1
    \$\begingroup\$ @SirBroccolia Not a problem, If you want me to recommend some good resources, I would say that you pick up a good C++ book or follow or follow this site. Moreover, this person on youtube has some amazing content for C++ topics \$\endgroup\$ – Aryan Parekh Oct 26 at 11:16
  • \$\begingroup\$ I'm currently reading C++ Primer and Principle and Practice Using C++ I'm still not fully understand about function, pointer, and class, thus I stop reading those books for the moment and making a project to help me understand more about those three things. Because of this project, I understand much more better about function. But, pointer and class still haven't got any clue. \$\endgroup\$ – Sir Broccolia Oct 26 at 12:14
  • 1
    \$\begingroup\$ @SirBroccolia yes! What made you doubt that structure? \$\endgroup\$ – Aryan Parekh Oct 26 at 12:19
  • 1
    \$\begingroup\$ @S.S.Anne Oops! Thanks for catching that mistake, I surely forgot to type program \$\endgroup\$ – Aryan Parekh Oct 26 at 18:31
6
\$\begingroup\$

As has been mentioned, your code is generally pretty good.

Turn on warnings, and fix them.

computerOrPlayer is supposed to return a bool, yet it doesn't always.

Unfortunately, by default C++ compilers do not warn about this undesirable mistake, but they do generally can detect it -- if you have activate the corresponding warnings.

For gcc and clang, my recommendation is to add the following flags to your command line: -Werror -Wall -Wextra. In details:

  • -Werror: treat warnings as errors.
  • -Wall: activate many warnings (not all, despite the name).
  • -Wextra: activate another batch of warnings (still not all).

Other options include using linters, such as cppcheck.

Compiler warnings and linters are like automated reviewers, they're invaluable, and much more responsive than humans.

What are your return types for?

Many of your functions return a bool, but often times you don't check the return value of your function calls.

You have to decide whether the function has important information to return, or not, and then stick to the decision:

  • If it has: then it should return a value, and this value should be checked at the call site.
  • If it has nothing to report: then it should not return anything (void).

The [[nodiscard]] attribute will enlist the help of the compiler to ensure that you don't forget to check a return value:

[[nodiscard]] bool yourfunction();

Use namespaces.

Defining symbols in the global namespace is not idiomatic in C++; the global namespace is already fairly crowded with all the C symbols, no need to add to the mess.

Instead it is recommended that each project have its own namespace, and possibly sub-namespaces if there are multiple modules -- though here it would be overkill.

namespace guessing_game {
}

What is public, what is private?

Your BracketingSearch.h exposes many signatures, but the client only uses one.

A well-defined module will typically expose only a subset of its types and functions -- this is its public interface -- and the rest should be "hidden", and inaccessible to the rest of the world.

In your case, we can see that main only ever calls startGame: it seems this is your public API, and anything else is an implementation detail.

In this case, the BracketingSearch.h header should only expose startGame: not the other functions, not the constants either.

The other functions and constants can be declared in private headers, which are only included either by other private headers, or by source files.

An example of organization:

include/
    guessing_game/            <-- matches namespace
        BracketingSearch.h
src/
    guessing_game/
        BracketingSearchImpl.hpp
        BracketingSearchImpl.cpp
        BracketingSearch.cpp

Then BracketingSearch.cpp will look like:

#include "guessing_game/BracketingSearch.h"
#include "guessing_game/BracketingSearchImpl.h"

namespace guessing_game {

void startGame() {
   ...
}

} // namespace guessing_game

And BracketingSearchImpl.cpp will look like:

#include "guessing_game/BracketingSearchImpl.h"

namespace guessing_game {

namespace {
    // ... constants ...
} // anonymous namespace

int randomNumGenerator(const int max, const int min)
{
    return rand() % max + min;
}

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

int rangeNum(const int max, const int min)
{
    return max - min;
}

// ... other functions ...

} // namespace guessing_game

And the interface is clear to uses -- they can only use what is declared in the (public) header.

Note: this public/private game is recursive; for example if randomNumGenerator is not used outside BracketingSearchImpl.cpp, then it should NOT be declared in BracketingSearchImpl.hpp and should be moved into the anonymous namespace.

Avoid global variables

Relying on global variables causes issues with testing, multi-threading, etc... it's best avoided.

In your case you relying on 3 global variables:

  1. The state of rand().
  2. std::cin.
  3. std::cout.

C++11 introduced the <random> header, which is the recommended way to generate random numbers, it will avoid your reliance on rand():

  • Passing the seed to startGame.
  • Use a distribution from the <random> header.

For the I/O streams, there are 2 possibilities:

  • Just take std::ostream& and std::istream& as argument to startGame.
  • Separate the I/O behind its own interface, and pass the interface to startGame.

Given the small scale of this game; I'd advise going with just passing the streams.

Note: when you are more comfortable with C++, you should look into Sans IO design, or Hexadecimal Architecture, the idea is that I/O should be moved to the edge of the application, and everything within the application should only interact with business-oriented interfaces. It goes hand in hand with Dependency Injection, too.

Tests

You should test your code.

As written it's hard to test due to the use of global variables; once they are removed (see previous point) it becomes much easier.

Testing will allow you to ensure that:

  • Invalid input is correctly handled.
  • Edge cases are correctly handled.
  • ...

And will give you more confidence that you are not breaking everything when changing your code.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is really a lot to think about. Thanks, Mate, for the advice! \$\endgroup\$ – Sir Broccolia Oct 26 at 21:20
  • \$\begingroup\$ I fixed a little error, you missed a s in [[nodiscard]] ([[nodicard]]) \$\endgroup\$ – Aryan Parekh Oct 28 at 6:58
  • 1
    \$\begingroup\$ @AryanParekh: Thanks! \$\endgroup\$ – Matthieu M. Oct 28 at 11:32
2
\$\begingroup\$

You have quite a nice structure. And while it is a bit much for this size of project, it's good training for bigger things.

Still, static const is strictly inferior where constexpr is a choice. Enum constants are also a nice option.

Marking parameters const can be useful for definitions of lengthier functions, which you commendably avoid. But for forward-declarations, especially in a header-file, they are just useless clutter grabbing attention better invested elsewhere.

Your range is curious:

  1. You are using a closed interval. That is rare in programming, and especially C++, as it is cumbersome and error-prone. Closed number-ranges may not be quite as rare as iterator- and pointer-ranges, but the same principle holds.
    And who would have thought, your calculation for the size of the range max - min + 1 is often off by one, which you partially compensate for with additional code.
  2. Giving the end before the beginning is very unexpected, not only in programming, especially using C++, but also for natural language, not that the latter is always a reliable guide.

rand() is generally an awful RNG. Which is not too surprising, considering it is often backwards-compatible to some antediluvian ancestor, and the standard interface is a bit restrictive. If you want a better one with more reliable quality, consider upgrading to <random>.

randomNumGenerator() is wrong. max is only the size of the output range if min is 1, generally it is (max - min + 1). Not that this method of mapping the randomness you have to the interval you need isn't generally dubious. There is a reason <random> also provides std::uniform_int_distribution.

Not sure what rangeNum() should calculate. If it should be the size of the range, it is wrong, see above. Anyway, fixing rangeNumToGuess() will eliminate the need for the one caller, allowing it to be pruned too.

I suggest making function-names actions: rangeNumGenerator() becomes getRandomNumber(), and rangeNumGuess() becomes guessNumber().

The argument to tolower() must be non-negative. And yes, that means you have to cast to unsigned char.
Actually, consider extracting a new function for getting a char from the user and transforming it to lower-case. You need it in at least two places, and only transform it in one, badly. That also allows you to eliminate a variable in both callers.

You could use switch in computerOrPlayer() too.

If a function always returns the same result, consider making it a void-function.

You have unrestricted recursion in restart().
Don't depend on the compiler to do tail-call optimization, especially as you forgot to return the result of the recursive call to make it a tail-call. At least there are no non-trivial dtors involved, but the escape-analysis involved might still be too much if even attempted.
Don't depend on the user to be too impatient to accumulate enough frames to cause a stack overflow.

main() has an implicit return 0; at the end. For whatever that is worth.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the advice, Mate! I will definitely keep it in mind for my next exercise \$\endgroup\$ – Sir Broccolia Oct 27 at 21:25

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.