C++: Mastermind game two players

I have made an attempt to write the Mastermind game, to improve my basics in C++. I would like my code reviewed, emphasising on design, readability and structure of the program.

The secret code is stored as a string of 4 character/PEGS and input is taken as: RRGY for red red green yellow.

#define RED "\x1b[31;1m"
#define GREEN "\x1b[32;1m"
#define YELLOW "\x1b[33;1m"
#define BLUE "\x1b[34;1m"
#define MAGENTA "\x1b[35;1m"
#define CYAN "\x1b[36;1m"
#define WHITE "\x1b[37;1m"
#define RESET "\x1b[0m"

void printCode(const std::string &code);
bool isMatching(const std::string &code, std::string &userCode);
inline void showMoves(std::vector &prevMoves);
void readCode(std::string &code);
void setup(unsigned int &maxGames, unsigned int &maxGuess);

const unsigned int PEGS = 4;

main()

```
int main()
{
unsigned int maxGuess, maxGames, p1score, p2score;
setup(maxGames, maxGuess);

for (int game = 0; game != maxGames; ++game)
{
bool won = false;
int score = 0;
std::string secretCode;
//keeps track of all the previous moves and feedback.
std::vector prevMoves;

std::cout << "New Game\nSet secret code: ";
readCode(secretCode);

for (int guess = 1; guess != maxGuess + 1; ++guess)
{
std::cout << "guess: " << guess << " / "
<< maxGuess << "\n";

std::string userCode;
std::cout << "Code: ";
readCode(userCode);
score++;

if (isMatching(secretCode, userCode))
{
won = true;
std::cout << WHITE << "Code successfully broken!\n";
printCode(secretCode);
break;
}

// Update previous moves and display them.
prevMoves.push_back(userCode);
showMoves(prevMoves);
}

if (!won)
{
std::cout << "Oops! You were unable"
<< " to c

I love the UI! Good job. But, there are a few (major) things that you can do better.

Where are the #includes?

Either you didn't paste them here (you should always post the whole code BTW), or you haven't included them. That's bad! Some compilers will happily compile your code even if you are missing some #includes, but that is not standard behavior.

When I compiled your code, I was greeted with a lot of errors, all related to missing headers.

Don't use macros when you can use variables

Macros are bad, but in some cases they are necessary, because there is no viable alternative. But not in your case. You could easily replace those "constants" with actual variables:

constexpr auto RED = "\x1b[31;1m";
constexpr auto GREEN = "\x1b[32;1m";
constexpr auto YELLOW = "\x1b[33;1m";
constexpr auto BLUE = "\x1b[34;1m";
constexpr auto MAGENTA = "\x1b[35;1m";
constexpr auto CYAN = "\x1b[36;1m";
constexpr auto WHITE = "\x1b[37;1m";
constexpr auto RESET = "\x1b[0m";

Don't declare variables at the top of functions if they are needed later

Always declare variables in the smallest scope possible, and never ever declare every variable used in a function at the top of it. That's bad for several reasons:

• You might initialize a variable that is expensive to create, only to not use it because the function returned prematurely.

• You might accidentally change a value of a variable that you shouldn't have changed.

• You might forget to initialize some variables (which you did - see isMatching), leaving them in an unspecified state. If you forget that the variable doesn't have a value yet, you will have a (possible) hard time debugging.

Listen to the compiler warnings!

Always listen to the compiler warnings, and always compile with a high warning level.

You have 3 warnings that can easily be fixed:

main.cpp: In function 'int main()':
main.cpp:29:29: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     for (int game = 0; game != maxGames; ++game)
                        ~~~~~^~~~~~~~~~~
main.cpp:40:35: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
         for (int guess = 1; guess != maxGuess + 1; ++guess)
                             ~~~~~~^~~~~~~~~~~~~~~
main.cpp:70:19: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
         if (score == maxGuess)

std::...::size_type is also known as std::size_t (within reasons)

std::size_t is required to be able to store the maximum size of any type. This means that std::size_t is either larger or equal to std::...::size_type. In fact, in most cases, size_type is a synonym:


When indexing C++ containers, such as std::string, std::vector, etc, the appropriate type is the member typedef size_type provided by such containers. It is usually defined as a synonym for std::size_t.

Taken from here.

This can save you some typing :)

Why inline?

Why is showMoves inline? It really doesn't need to be inline, as it is declared inside a translation unit, and thus can't be used anywhere else, which is the whole point of inline! You should declare a function as inline to enable the compiler to better optimize it as the definition of the function will be available in every translation unit using it.

Contrary of what you might think, declaring a function inline doesn't make the compiler inline your function. It does this whether you declared it inline or not. It can also ignore the inline keyword, as it might deem the function unsuitable for inlining.

Catch exceptions by const&

If you don't catch exceptions by reference, you will see unexpected behavior if an exception is thrown that inherits that specific exception. This is called object slicing. Making it const is better because it prevents you have actually modifying the exception if you didn't mean to (this can be applied to every variable - see const correctness).

Prefer returning a value than passing a reference

You should write functions that return a new value, instead of modifying a value passed as a parameter. The reason being, is that you never want your variables to be in an unspecified state, and if you pass be ref, you will need to write 2 lines instead of 1 when initializing it, possibly leaving it uninitialized for a short amount of time.

// 2 lines
std::string secretCode;
readCode(secretCode);

// 1 line
std::string secretCode = readCode();

With the advent of move semantics and RVO, there won't be any string copied, it will be moved, which is very cheap. You can read more here.

Use < instead of !=

In every loop that you have, you have var != end or similar constructs. If you accidentally increment var past end, the loop will continue running. Instead, if you would have used var < end, then the loop will still terminate.

Sometimes, this is not possible, for example with iterators, which don't have an operator<.

You can i

StackExchange Code Review Q#157556, answer score: 6