2
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ You're right about the first one. I should refactor that function so it will return the opposite. About the return, I like to have only 1 return, makes the code look cleaner, only 1 exit point, etc... About the third error, I'm using namespace std, so that should be fine, right? \$\endgroup\$ – alexandernst Oct 7 '15 at 11:43
  • 1
    \$\begingroup\$ @alexandernst Yes, that should work. It will even work without 'using namespace' on most compilers, but that's not standard, it just happens to work because usually compilers reuse the same header files for C and C++. Well, having a single return is a hotly debated topic, but not going into that, your function already has multiple returns, so I guess it wouldn't make a difference... \$\endgroup\$ – glampert Oct 7 '15 at 14:12

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.