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.

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-&gt;canMoveForward(bytes_with_skip) == false) return false;

//Read the required number if bits. Ignore the skip_b_bits argument.
Bits *data = this-&gt;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 &lt; check_n_bits) return false;

unsigned int last_bits = check_n_bits &lt;= 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 &lt; 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-&gt;read_uint8();
    sprintf((char *) &amp;tmp_bin_repr, BYTETOBINARYPATTERN, BYTETOBINARY(c));
    for(int j = 0; j &lt; bits_to_compare; j++) {
        tmp_bin_repr_2[j] = *(&amp;(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 &amp; 0x80 ? 1 : 0), \
    (byte &amp; 0x40 ? 1 : 0), \
    (byte &amp; 0x20 ? 1 : 0), \
    (byte &amp; 0x10 ? 1 : 0), \
    (byte &amp; 0x08 ? 1 : 0), \
    (byte &amp; 0x04 ? 1 : 0), \
    (byte &amp; 0x02 ? 1 : 0), \
    (byte &amp; 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-&gt;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?
Bug

Your computation of last_bits is wrong:


unsigned int last_bits = check_n_bits &lt;= 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 *) &amp;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] = *(&amp;(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(&amp;tmp_bin_repr[skip_bits_if_last], &amp;bin_string[i*8], bits_to_compare) != 0) {