Program that guesses your number using bitwise operations

Question

I got the idea for this program from this site.

functions.cpp:

#include <iostream>
#include <cstdlib>
#include <iomanip>

namespace my
{
    int getOneOrZero()
  {
    return (rand() >> 14);   // >> the bitwise operator
  }

// getArray leaves the numbers ( 0-99 ) that have in their bit position        represented by bigFlag the num ( 0 or 1 )

void printNums(int8_t bitFlag, int num)    // num is from getOneOrZero() so it's 1 or 0;
{
    for (int counter = 0; counter < 100; ++counter)  // there are quite a few implicit conversions to one byte integers
        {
            if ( (counter & bitFlag) != num*bitFlag )
                std::cout << std::setw(8) << " ";
            else
                std::cout << std::setw(8) << counter;

            if ( (counter % 9) == 0)
                std::cout << std::endl;
        }
}

char getAnswer()
{
    while (1)
    {
        std::cout << "\n Is your number shown above ?\n\n 'y' for yes , 'n' for no , 'r' for reset : ";
        char answer;
        std::cin >> answer;
        std::cin.ignore(32767,'\n');

        if (std::cin.fail())
            std::cin.clear();

        if (answer == 'y' || answer == 'n' || answer == 'r')   // I could use switch but nevermind
            return answer;
    }
}

void turnsPassed(int turns)
{
    std::cout << "\n This is turn " << turns << "\n\n";
}

int swapZeroOrOne(int num)
{
    switch (num)
    {
    case 0 :
        return 1;
    case 1 :
        return 0;
    default:
        std::cout << "\nSwapZeroOrOne ERROR !\n";
        break;
    }
}

int getUpdateForGuessNum(int8_t flag, int num, char answer)
{
    switch (answer)
    {
    case 'n' :
        { int newNum = swapZeroOrOne(num);
          return newNum*flag;
        }
    case 'y' :
        return num*flag;
    case 'r' :
        break;
    default  :
        std::cout << "\n ERROR ! In getUpdateForGuessNum\n";
        break;
    }
  }
}

main.cpp:

#include <iostream>
#include <iomanip>
#include <cstdlib>
#include <ctime>
#include "functions.hpp"
#include "constants.hpp"
#include <stdlib.h>      // for system() commands >.<
int main()
{
srand( static_cast<unsigned int>(time(0)));

Reset :      // goto !
system("cls");

int guessNum = 0;

// guessing loop !

for (int counter = 0; counter < 7; ++counter)
{
    if (counter == 0)
        std::cout << "\n Think of a number between 0 to 99\n";

    my::turnsPassed(counter + 1);  // +1 cause counter starts from 0

    int num {my::getOneOrZero()};  // this gives a randomness to the numbers shown each time you run the programm

    my::printNums(myVar::bitFlag[counter],num);

    char answer { my::getAnswer() };

    if (answer == 'r')
        goto Reset;

    guessNum |= my::getUpdateForGuessNum(myVar::bitFlag[counter],num,answer);

    system("cls");
}

std::cout << "\n Your number is \n";
std::cout << "\n" << std::setfill('-')  << std::setw(81) << "\n";
std::cout << std::setfill(' ') << std::setw(41) << guessNum << "\n";
std::cout << "\n" << std::setfill('-')  << std::setw(81) << "\n";

system("pause");  // I should fix this

return 0;
}

functions.hpp:

#pragma once

namespace my
{
    int getOneOrZero();
    void printNums(int8_t,int);
    char getAnswer();
    void turnsPassed(int);
    int getUpdateForGuessNum(int8_t,int,char);
    int swapZeroOrOne(int);

}

constants.hpp:

#pragma once

namespace myVar
{
    const int8_t bitFlag[] {0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80};
}

Please suggest ways to improve this program. Explain the suggestions you make because I need to know why I should change something.

Answer 1

Here are some things that may help you improve your code.

Understand type implications

The code includes a bitFlag array that is declared as const int8_t but then contains a value that is 0x80. The problem with that is that when the compiler encounters the constant 0x80, it converts it into an int by default, and so that would be the value 128. However, 128 is not representable in an int8_t. That bit pattern is actually -128 as an int8_t, so I'd recommend either using a different type (such as uint8_t) for the variable or writing -0x80 for the constant which is correct, but a little strange looking.

Know the standard types

This function is in the current code:

int getOneOrZero()
{
    return (rand() >> 14);   // >> the bitwise operator
}

There are two problems with this. The first is that it apparently assumes that an int is 16 bits. However, on my machine, an int is 64 bits. In general, you can't assume that the size of an int is a fixed size. The standard only says (implicitly from the required range) that it must be 16 bits, but it may be larger. The second problem is addressed in the next point.

Consider using a better random number generator

Because you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::bernoulli_distribution and friends in the <random> header.

Here's one way to rewrite it:

int getOneOrZero()
{
    static std::mt19937 gen{std::random_device{}()};
    static std::bernoulli_distribution bd;
    return bd(gen);   
}

Ensure every control path returns a proper value

The swapZeroOrOne routine returns 1 or 0 under some set of conditions but then doesn't return anything at all otherwise (although it prints an error essage). This is an error because all control paths should return a value. Since it's only used once, and because it can be trivially rewritten, I'd probably replace this:

case 'n' :
    { int newNum = swapZeroOrOne(num);
      return newNum*flag;
    }

with this:

    return (1-num)*flag;

Avoid using goto

Having a goto statement in modern C++ is usually a sign of bad design. Better would be to eliminate them entirely -- it makes the code easier to follow and less error-prone. In this code, it's probable that you could use a loop instead:

for (bool playing=true; playing;  )
    // code to play the game
    // ask user if they want to play again
    playing = answer == 'r'; 
}

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    std::cout << "\x1b[2J";
}

Think of the user

If the user were to get an error message that said:

ERROR ! In getUpdateForGuessNum

What use is that unless they also happen to be the author of the code? Instead, for errors that are an indication of a program flaw, I'd use assert or for things that are unusual but still should be accomodated by the program, use an exception.

Use objects

You have a guessNum and counter to support the game and then separate functions printNums and getAnswer, etc. that operate on guessNum. With only a slight syntax change, you would have a real object instead of C-style code written in C++. You could declare a GuessNum object and then printNums, getAnswer, etc. could all be member functions.

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Answer 2

Some more besides @Edward's brilliant points:

Do not refactor simple oneliners into functions unnecessarily

void turnsPassed(int turns)
{
    std::cout << "\n This is turn " << turns << "\n\n";
}

This just obfuscates the output statement for a reader and doesn't add any value over writing that std::cout directly in the context (especially since it's called only at one place).

Do not introduce unnecessary namespaces

Namespaces should be used to group logically and semantically related code together.

There's not much value of separating

namespace myVar
{
    const int8_t bitFlag[] {0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80};
}

into it's own namespace myVar, or even have that declared in a separate header file.

_{Also the type should be uint8_t, since 0x80 would yield a negative value otherwise.}

It should appear in the same namespace and along with the functions that make use of this array.

In fact it would be better (as mentioned) in the other answer to place everything in a class:

class NumberGuessingGame
{
public:
    int getOneOrZero();
    void printNums(int8_t,int);
    char getAnswer();
    void turnsPassed(int);
    int getUpdateForGuessNum(int8_t,int,char);
    int swapZeroOrOne(int);

private:
    const uint8_t bitFlag[] {0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80};
}

Also take care to give namespaces or classes resonable and helpful naming for a future reader of your code. my and myVar do not have any value regarding that.

Do not use std::cin.ignore() unnecessarily

std::cin.ignore(32767,'\n');

This statement is probably meant to catch up cases when the user made an input of multiple characters in a single line.

First problem is the magic number 32767, using something like std::limits<size_t>::max would be more portable and better to read again.

Also you wouldn't need that whole stuff if you use a std::string and std::getline() to realize that function. You could rewrite that whole function much simpler then:

char getAnswer()
{
    while (1)
    {
        std::cout << "\n Is your number shown above ?\n\n 'y' for yes , 'n' for no , 'r' for reset : ";
        std::string answer;
        std::getline(std::cin,answer);

        if (answer[0] == 'y' || answer[0] == 'n' || answer[0] == 'r')
            return answer[0];
        else
            std::cout << "Please enter one of 'y', 'n' or 'r';
    }
}

Checking for fail() is not needed since there's only char or string input expected anyways.