1
\$\begingroup\$

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.

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

//Check if `this` contains the number of bytes we need
if(this->canMoveForward(bytes_with_skip) == false) return false;

//Read the required number if bits. Ignore the skip_b_bits argument.
Bits *data = this->readBits(check_n_bits, skip_b_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;

unsigned int last_bits = check_n_bits <= 8 ? check_n_bits : (bytes * 8) - check_n_bits;
last_bits = last_bits == 0 ? 8 : last_bits;

char tmp_bin_repr[9], tmp_bin_repr_2[9];

for(size_t i = 0; i < bytes ; i++) {
    int skip_bits_if_last = i + 1 == bytes ? (8 - last_bits) : 0;
    int bits_to_compare = i + 1 == bytes ? last_bits : 8;

    uint8_t c = data->read_uint8();
    sprintf((char *) &tmp_bin_repr, BYTETOBINARYPATTERN, BYTETOBINARY(c));
    for(int j = 0; j < bits_to_compare; j++) {
        tmp_bin_repr_2[j] = *(&(bin_string[i * 8]) + j);
    }

    if(memcmp((const char *) tmp_bin_repr + skip_bits_if_last, (const char *) tmp_bin_repr_2, bits_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)

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\$
  • \$\begingroup\$ This looks suspicious: sprintf((char *) &tmp_bin_repr, .... You are actually passing the address of the array, not the array itself, which appears to work, but would fail to compile without that cast. Is that intentional? Why? \$\endgroup\$ – glampert Sep 29 '15 at 3:03
2
\$\begingroup\$

Bug

Your computation of last_bits is wrong:

unsigned int last_bits = check_n_bits <= 8 ? check_n_bits : (bytes * 8) - check_n_bits;
last_bits = last_bits == 0 ? 8 : last_bits;

Suppose check_n_bits were 18 as in your example. This would make bytes 3, and so last_bits would be set to 24 - 18 = 6 instead of 2. Later, you would check 6 bits in the 3rd byte instead of 2 bits. This must mean that your unit tests aren't covering enough cases.

Awkward array usage

There are a couple of lines that are really awkward:

sprintf((char *) &tmp_bin_repr, BYTETOBINARYPATTERN, BYTETOBINARY(c));

Here you are casting the address of a character array into a character pointer. Any time you use a cast, you should think about whether you really need that cast or if you are simply doing something wrong. In this case, you are doing it wrong. You can just pass the array like this:

sprintf(tmp_bin_repr, BYTETOBINARYPATTERN, BYTETOBINARY(c));

The second awkward line is this:

tmp_bin_repr_2[j] = *(&(bin_string[i * 8]) + j);

which can be simplified to this:

  tmp_bin_repr_2[j] = bin_string[i*8 + j];

No need for temp copy

Actually I don't think you really even need tmp_bin_repr_2 because all you are doing is making a copy of part of bin_string. You could just compare with that part of bin_string directly if you modified your call to memcmp(). You can also remove the unnecessary casts as well:

if (memcmp(&tmp_bin_repr[skip_bits_if_last], &bin_string[i*8], bits_to_compare) != 0) {
\$\endgroup\$
  • \$\begingroup\$ You're right! Let me fix the bug and apply your suggestions. Wi'll post back the result in a moment. \$\endgroup\$ – alexandernst Sep 29 '15 at 10:56
  • \$\begingroup\$ I think... there is no bug. last_bits is used in skip_bits_if_last and bits_to_compare to calculate the amount of bits to compare. Or maybe I'm misreading my code? \$\endgroup\$ – alexandernst Sep 29 '15 at 11:15
  • \$\begingroup\$ @alexandernst Step through your code and see what happens. skip_bits_if_last is set to 2 and bits_to_compare is set to 6, but I think those should be reversed. \$\endgroup\$ – JS1 Sep 29 '15 at 17:30
  • \$\begingroup\$ Indeed, you're right. I simplified my code. Should I edit my question, remove the old code and post the new one or append it to the old code? \$\endgroup\$ – alexandernst Sep 30 '15 at 17:05
  • 1
    \$\begingroup\$ @alexandernst Here at code review, you shouldn't edit the question after you receive answers. If you want a followup review, you may post a new question with the updated code and add "followup" somewhere in the question title. \$\endgroup\$ – JS1 Sep 30 '15 at 17:16

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.