Comparing random number of bits (followup)

Question

This is a follow-up of Comparing random number of bits.

I have a function that compares bits from a binary representation string and a chunk of memory and returns true if they are equal.

This is the function (which works as expected): I'll explain with comments what some external methods are supposed to do, accept that they work as expected for the sake of this question.

bool Bits::compareBinary(const char *string, size_t check_n_bits, size_t skip_n_bits){
    bool match = true;

    size_t bytes = (check_n_bits + 7) / 8;
    size_t bytes_with_skip = (check_n_bits + skip_n_bits + 7) / 8;

    if(this->canMoveForward(bytes_with_skip) == false) return false;

    Bits *data = this->readBits(check_n_bits, skip_n_bits);

    unsigned char *bin_string = Utils::removeSpaces(string);
    size_t len = strlen((const char *) bin_string);
    if(Utils::isValidBinString(bin_string) || len < check_n_bits) return false;

    char tmp_bin_repr[9];

    for(size_t i = 0; i < bytes; i++) {
        int chars_to_compare = i + 1 == bytes ? check_n_bits % 8 : 8;
        chars_to_compare = chars_to_compare == 0 ? 8 : chars_to_compare;

        uint8_t c = data->read_uint8();
        sprintf(tmp_bin_repr, BYTETOBINARYPATTERN, BYTETOBINARY(c));

        if(memcmp(tmp_bin_repr, &bin_string[i * 8], chars_to_compare) != 0) {
            match = false;
            break;
        }
    }
}

Helper macro:

#define BYTETOBINARYPATTERN "%d%d%d%d%d%d%d%d"
#define BYTETOBINARY(byte) \
    (byte & 0x80 ? 1 : 0), \
    (byte & 0x40 ? 1 : 0), \
    (byte & 0x20 ? 1 : 0), \
    (byte & 0x10 ? 1 : 0), \
    (byte & 0x08 ? 1 : 0), \
    (byte & 0x04 ? 1 : 0), \
    (byte & 0x02 ? 1 : 0), \
    (byte & 0x01 ? 1 : 0)

Let's call this function:

compareBinary(const char *string, size_t check_n_bits, size_t skip_b_bits)

As there are a couple of methods and a class I can't post here (for the sake of this question), you're free to explore the entire code here. How is this function used? The function is a method of an object that contains some data. When this method is called, it compares as many bits from a binary string representation of data as we pass it with the data contained in the object.

Example:

mObj->compareBinary("01010101 11110100 00", 18, 0);

This will compare 18 bits in total with the data that is hold in the object. First 8 chars of the passed string will be taken and the first byte of the data hold by the object will be converted to a string (using the helper macro). Then both will be compared. Second 8 chars, the same. Then we'd get the last 00 chars, take the third byte, convert it to a 8 chars binary representation, but only the first two chars will be compared.

This lets me compare a random number of bits with a simple binary representation of the data.

While the function works as expected and it's passing multiple unit tests, I believe it can be refactored and optimized. Can you give me any tips how to do it?

Answer 1

The code looks fine, I can only mention a few things more related to code style, but before we get to that, this one line caught my attention:

if(Utils::isValidBinString(bin_string) || len < check_n_bits) return false;

Nothing really wrong with it, but it reads weird. It returns false if the binary string "is a valid bin string". Hum? Why would we want to fail if the string isValidBinString? This statement is either missing a ! at the beginning, or the function does the exact opposite of what its name advertises, unless you are actually wanting to check "invalid strings", which would also be weird...

---

As said before, I don't see anything particularly bad with the code, but one thing that I would refrain from doing is prefixing in-class member access with the this-> pointer. That has no practical purpose other than visual noise, but it can also hide variable names that shadow each other, which in turn is a thing you should really avoid.

You could make more thorough use of const on locals that are only assigned once, to ensure they are not changed later. That's a good habit to acquire. We can easily track the lifetime of a variable in a small scope, but sometimes scopes grow big, so it is nice to have a guarantee that a variable is not changed after its initialization. Languages like Rust make immutable the default, which is actually a neat idea, most of time you just want a single assignment.

Storing the return value here:

if(memcmp(tmp_bin_repr, &bin_string[i * 8], chars_to_compare) != 0) {
    match = false;
    break;
}

Is pointless since the function is over after the loop. Just return false directly.

A minor issue, but strict C++ mandates that C library functions and types be accessed from the std:: namespace, and imported from header files like <cstdio>, <cstdlib>, etc. To comply with that and ensure portability across different C++ compilers, you have to, for instance, use std::size_t, std::strlen(), etc.

I don't have problems with macros when they are well used, but you could have accomplished BYTETOBINARYPATTERN and BYTETOBINARY using a constant and inline function, respectively, gaining scoping and better error messages if misused.