Learn You A Hex

Question

This small program generates random operands and operations and asks the user to perform the computation and input the result. I want to make it more C++ and remove any C-ness it might inappropriately contain. How to go about it?

/*
 * https://artfulcode.wordpress.com/learn-you-a-hex/
 * https://github.com/cmovz/
 * License: Use it as you wish, just keep this notice.
 */

#include <iostream>
#include <string>
#include <random>
#include <cstdlib>
#include <limits>

enum Ops {
    ADD, SUB, MUL, DIV, OPS_COUNT
};

static char const* ops_signs[] = {"+", "-", "*", "/", "?"};
static char const* bin_to_hex_table = "0123456789abcdef";

static char const hex_to_bin_table[256] = {
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x1,0x2,0x3,0x4,0x5,
    0x6,0x7,0x8,0x9,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xA,0xB,0xC,0xD,0xE,0xF,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xa,0xb,0xc,0xd,0xe,0xf,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
    0x0,0x0,0x0,0x0
};

std::string to_hex(unsigned long long hex)
{
    std::string rev_s;

    do {
        rev_s.push_back(bin_to_hex_table[hex&0xf]);
    } while(hex >>= 4);

    std::string s = "0x";
    s.insert(s.end(), rev_s.rbegin(), rev_s.rend());

    return s;
}

long long hex_to_bin(std::string const& str)
{

    long long bin = 0;
    int shift = 0;

    for(std::string::const_reverse_iterator ite = str.rbegin();
        ite != str.rend();
        ++ite)
    {
        bin |= static_cast<long long>(
                    hex_to_bin_table[static_cast<unsigned char>(*ite)]
                )
                << shift;
        shift += 4;
    }

    return bin;
}

bool is_hex(std::string const& str)
{
    for(std::string::const_iterator ite = str.begin();
        ite != str.end();
        ++ite)
    {
        unsigned char c = *ite;
        if('0' != c && !hex_to_bin_table[c]){
            return false;
        }
    }
    return true;
}

void bye()
{
    std::cout << std::endl << "------------" << std::endl << std::endl
              << "Bye!" << std::endl;

    std::exit(0);
}

int main()
{
    std::random_device hw_rand;
    unsigned int mask;

    std::cout << "=============================" << std::endl
              << " Welcome to Learn You A Hex!" << std:: endl
              << "=============================" << std::endl
              << std::endl;

    if(hw_rand.entropy() == 0){
        std::cout << "INFO: Using software-generated random numbers"
                  << std::endl;
    }
    else {
        std::cout << "INFO: Using hardware-generated random numbers"
                  << std::endl;
    }

    std::cout << std::endl;

    while(true){
        unsigned int digits;
        std::cout << "How many digits you want? [1;8]" << std::endl;

        std::cin >> digits;
        if(!std::cin){
            if(std::cin.eof()){
                bye();
            }

            std::cout << "Error: bad input" << std::endl;
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }

        else
        if(digits >= 1 && digits <= 8){
            mask = 0;
            while(digits--){
                mask |= 0xf << (digits * 4);
            }

            break;
        }

        std::cout << "Range must be [1;8]" << std::endl;
    }

    do {
        long long correct_answer;
        long long user_answer;
        unsigned int a = hw_rand() & mask;
        unsigned int b = hw_rand() & mask;
        unsigned int op = hw_rand() % OPS_COUNT;
        std::string answer_str;

        switch(op){
        case ADD:
            correct_answer = (long long)a + b;
            break;

        case SUB:
            correct_answer = (long long)a - b;
            break;

        case MUL:
            correct_answer = (long long)a * b;
            break;

        case DIV:
            if(b == 0){
                continue; // can't divide by 0
            }
            correct_answer = (long long)a / b;
            break;
        }

        std::cout << "------------" << std::endl
                  << to_hex(a) << " " << ops_signs[op] << " "
                  << to_hex(b) << " = ?" << std::endl
                  << "0x";

        while(std::cin >> answer_str){
            if(!is_hex(answer_str)){
                std::cout << "Error: not hex, write only the hex digits"
                          << std::endl;
            }
            else
            if(answer_str.size() > sizeof(long long) * 2){
                std::cout << "Error: input too long, hex number should not "
                             "exceed " << sizeof(long long) * 2 << " digits"
                          << std::endl;
            }
            else {
                user_answer = hex_to_bin(answer_str);
                break;
            }

            std::cout << "0x";
        }

        if(!std::cin){
            bye();
        }

        if(user_answer == correct_answer){
            std::cout << "Correct!" << std::endl;
        }
        else {
            std::cout << to_hex(correct_answer) << std::endl;
        }
    } while(true);
}

Answer 1

This is easy to read and very straightforward. You handle errors which is a plus, too! Nice work! Here are some things that I think would make it even better.

Simplify

Some of the code is very simple which is great. But a few things are done in fairly inefficient ways. The most obvious is that huge table of hex digits (hex_to_bin_table). It's very easy to have a bad value in there. Good luck finding it if you do! I'd remove the table and write a simple function to do the conversion. Something like this:

unsigned long long hex_char_to_bin(char hex)
{
    unsigned long long result = 0;
    char lower_hex = tolower(hex);
    if (('0' <= lower_hex) && (lower_hex <= '9'))
    {
        result = static_cast<unsigned long long>(lower_hex - '0');
    }
    else if (('a' <= lower_hex) && (lower_hex <= 'f'))
    {
        result = static_cast<unsigned long long>(lower_hex - 'a' + 10);
    }
    return result;
}

This converts the character to lowercase, eliminating the need to check for both "A"-"F" and "a"-"f". Then it checks if it's a number, and if so, subtracts the character '0' from it. That will give a value between 0 and 9 for the digit between 0 and 9. If it's an 'a' through an 'f' character, it will subtract off 'a', giving a value between 0 and 5, and then it adds 10 to it, giving 10-15 (or hex 0xa through 0xf).

Now the table is no longer needed and your hex_to_bin() function looks like this:

long long hex_to_bin(std::string const& str)
{

    long long bin = 0;
    int shift = 0;

    for(std::string::const_reverse_iterator ite = str.rbegin();
        ite != str.rend();
        ++ite)
    {
        bin |= hex_char_to_bin(*ite) << shift;
        shift += 4;
    }

    return bin;
}

You could also remove the type declaration of ite and replace it with auto if you're using C++11 or later.

The is_hex() function could use some simplifying, too. It turns out there's an STL function for checking if a digit is a hex digit. It's called std::isxdigit(). Using that, then your is_hex() method becomes:

bool is_hex(std::string const& str)
{
    for(std::string::const_iterator ite = str.begin();
        ite != str.end();
        ++ite)
    {
        if (!std::isxdigit(*ite))
        {
            return false;
        }
    }
    return true;
}

Naming

It seems like I'm always harping on people about naming things. It has been declared one of the hardest things to do well in computer programming, so I'm not alone. I think your function names could be improved a bit. For example, to_hex takes an unsigned long long named hex. It seems like it's already in hex. And it returns a std::string. Maybe it could be renamed to_hex_string(unsigned long long value)? Since you're converting it to a string, specifically. (And I don't love the name value, but in this case it doesn't actually represent anything concrete, so even naming it x would be fine.)

Likewise, hex_to_bin() sounds like it converts from hexadecimal to binary. But it doesn't - it converts from a string to a numerical value. I'd call it hex_string_to_number().

And it might seem like nitpicking, but I'd call is_hex() something like is_valid_hex_string().

Functions

I would definitely take the logic in main() and break it out into functions. Just as you have the bye() function, you could make a print_intro_text() function to display the introductory text.

The loop which gets the number of digits the user wants could be put into a function called get_num_digits().

The main loop could go into a function like present_quiz(). I'd break out the while loop that gets the user's input into a separate function, as well. Something like get_answer().

Unnecessary Infinite Loops

You have at least 2 infinite loops in your code, to which you've added special code inside the loop to break out of the loop if some condition is met. This is difficult to follow and unnecessary. You should just make the loop condition be the condition that you're currently breaking on. The first loop (in main()) could easily be rewritten to something like this:

bool is_input_valid = false;
while (!is_input_valid)
{
    //...get the number of digits
    if ((1 <= digits) && (digits <= 8)
    {
        //... mask stuff
        is_input_valid = true;
    }
    if (!is_input_valid)
    {
        std::cout << "Range must be [1;8]" << std::endl;
    }
}

This is much easier to follow.

The second loop in main() is even worse! There is no exit condition! The only way out is to force-quit the program. Are you sure your users know how to do that? Why not just put a prompt at the end that asks if they want to try again or quit?

Removing C-ness

I'll be honest - I don't see a lot of C-ness here. You aren't using C strings. You aren't calling any C standard library functions that have C++ equivalents that are better to use. You're using iterators where appropriate. (You could use range-based loops in C++11, but it's not any more C++-ish than using iterators.) This doesn't really strike me as a task that justifies creating new classes.

You do have a few standard C casts that would be better as static_casts. Also, you tend to declare your variables at the top of their scope. It's better to declare them on the line before you use them, when possible. (It's not possible with the switch statement, for example.) But beyond that, it seems perfectly fine as C++ code. In fact, I even commented out your inclusion of cstdlib and limits and it compiled just fine.

Answer 2

Conversion decimal ⇋ hex

The character-by-character conversion is a lot of code that needs testing, and it makes non-portable assumptions about the system's character coding. We can use string streams to do the work for us:

#include <iomanip>
#include <sstream>

std::string to_hex(unsigned long long hex)
{
    std::ostringstream out;
    out << std::showbase << std::hex << hex;
    return out.str();
}

long long hex_to_bin(std::string const& str)
{
    long long bin = 0;
    std::istringstream in(str);
    in >> std::hex >> bin;
    return bin;
}

bool is_hex(std::string const& str)
{
    long long bin;
    std::istringstream in(str);
    in >> std::hex >> bin;
    return in.eof();
}

(Those function names can be improved; see other answers for suggestions.)

You can easily combine the testing for a valid hex string with extracting the value (e.g. by returning a pair, [is_valid, value]).

Avoid negative results

This exchange surprised me:

0x4 - 0x9 = ?
0x-5
Error: not hex, write only the hex digits
0x5
0xfffffffffffffffb

This could be avoided by testing the arguments and using std::swap to exchange them if necessary.

Use switch to match names to enum values

It's easy to overlook a mismatch in lists like this, especially when the list gets longer:

enum Ops {
    ADD, SUB, MUL, DIV, OPS_COUNT
};

static char const* ops_signs[] = {"+", "-", "*", "/", "?"};

A clearer way to achieve the same result is

enum Operator : unsigned int {
    ADD, SUB, MUL, DIV
};
const auto OPS_COUNT = DIV + 1;

const char *ops_sign(Operator op)
{
    switch (op) {
    case ADD: return "+";
    case SUB: return "-";
    case MUL: return "*";
    case DIV: return "/";
    }
    // not reached
    return "??";
}

Make sure your compiler is set to warn about missing case statements when switching on enum values (for GCC, that's -Wswitch; that's included in -Wall, which I recommend you always use).

Consider using an array of operation objects

You might consider this suggestion overkill, but I claim it's instructive.

Object-oriented coding tends to look at switch as a failure to express intent using objects. With that in mind consider that the supported operations are a collection of objects with behaviours, and it's our job to choose one of those behaviours at random. Then we can create an abstract base class and a collection of instances:

struct Operation {
    unsigned int a = 0;
    unsigned int b = 0;

    virtual bool args_ok()
    {
        return true;
    }
    virtual std::string name() const = 0;
    virtual unsigned int answer() const = 0;
    virtual ~Operation() {}
};

struct Addition : Operation
{
    std::string name() const override { return "+"; }
    unsigned int answer() const override { return a + b; }
};

struct Subtraction : Operation
{
    bool args_ok() {
        if (a < b) std::swap(a, b); // ensure positive result
        return true;
    }
    std::string name() const override { return "-"; }
    unsigned int answer() const override { return a - b; }
};

struct Multiplication : Operation
{
    std::string name() const override { return "*"; }
    unsigned int answer() const override { return a * b; }
};

struct Division : Operation
{
    bool args_ok() {
        return b != 0;          // avoid division by zero
    }
    std::string name() const override { return "/"; }
    unsigned int answer() const override { return a / b; }
};

static const std::unique_ptr<Operation> operations[] = {
    std::make_unique<Addition>(),
    std::make_unique<Subtraction>(),
    std::make_unique<Multiplication>(),
    std::make_unique<Division>(),
};

We can then pick and use one of them like this (look, no switch!):

    auto& op = operations[hw_rand() % std::size(operations)];
    op->a = hw_rand() & mask;
    op->b = hw_rand() & mask;

    if (!op->args_ok())
        continue;

    auto const correct_answer = op->answer();

    std::cout << "------------" << std::endl
              << to_hex(op->a) << " " << op->name() << " "
              << to_hex(op->b) << " = ?" << std::endl
              << "0x";

    long long user_answer;
    std::string answer_str;

Use the correct stream for purpose

In all the following, std::cerr would be more appropriate than std::cout:

        std::cout << "Error: bad input" << std::endl;

    std::cout << "Range must be [1;8]" << std::endl;

            std::cout << "Error: not hex, write only the hex digits"
                      << std::endl;

            std::cout << "Error: input too long, hex number should not "
                         "exceed " << sizeof(long long) * 2 << " digits"
                      << std::endl;

Answer 3

Perhaps you could start with a class that encapsulates and hides your data tables and then exposes the functions as member functions and move the further ideas that have come to your mind in main into your new class as well and then refactor from there?