3
\$\begingroup\$

My implementation of a binary-to-bits converting function:

#include <stdio.h>
#include <limits.h>

unsigned char *return_byte (unsigned char source)
{
    static unsigned char byte[CHAR_BIT];
    unsigned char *p = byte, i;

    for(i = 0; i < CHAR_BIT; i++)
        p[i] = '0' + ((source & (1 << i)) > 0);

    return p;
}



int main(void)
{
    unsigned char val = 200;

    printf("Value:\t%i\nBinary:\t%s", val, return_byte(val));

    return 0;
}

Output:

Value:  200
Binary: 00010011
\$\endgroup\$
  • \$\begingroup\$ I just received an idea for optimization. Make the function understands of endians. But I will leave this to the reviewers. \$\endgroup\$ – Genis Feb 22 '15 at 22:18
  • 2
    \$\begingroup\$ You could provide a self-review. \$\endgroup\$ – Hosch250 Feb 22 '15 at 22:22
  • \$\begingroup\$ Can I actually do that? \$\endgroup\$ – Genis Feb 22 '15 at 22:34
  • 1
    \$\begingroup\$ Yes, you can - especially if it dramatically improves the code. \$\endgroup\$ – Hosch250 Feb 22 '15 at 22:35
  • \$\begingroup\$ Your output is (of course) little-endian. That's not the conventional way to write numbers. We usually write big-endian style even in non-decimal bases. 200 is 'two hundred' not 'two'. I would normally read binary '00010011' as nineteen (decimal). \$\endgroup\$ – user59064 Feb 23 '15 at 14:06
3
\$\begingroup\$
  • return_byte doesn't tell much what the function is intended to do. btoa would be more in line with C naming convention (akin to itoa)

  • A p variable is pretty much useless. Consider

    static unsigned char byte[CHAR_BIT];
    unsigned char i;
    
    for(i = 0; i < CHAR_BIT; i++)
        byte[i] = '0' + ((source & (1 << i)) > 0);
    
    return byte;
    
  • Using an unsigned char for indexing is questionable. In any case it doesn't buy you much against a native int.

  • A boolean expression as + operand may raise some eyebrows (true is guaranteed to be a non-zero in an arithmetic context, but it is not guaranteed to be 1). I recommend to force an arithmetic value by right-shifting source instead of left-shifting the mask:

    for(i = 0; i < CHAR_BIT; i++) {
        byte[i] = '0' + (source & 1);
        source >>= 1;
    }
    
  • You do not null-terminate the resulting string. The fact that your test succeeded is a sheer (un)luck.

  • You probably know that returning static makes the code non-reentrant and thread-unsafe. You may want to let client provide a space for a resulting string.

\$\endgroup\$
  • \$\begingroup\$ I don´t think I need to null-terminated with the use of static, which will automatically enzero all the bytes. Everything other than that seems fairly legal. \$\endgroup\$ – Genis Feb 22 '15 at 23:02
  • \$\begingroup\$ It will "enzero" CHAR_BIT bytes (and we will overwrite them at the very first call anywhay). What happens with the next byte beyond the array (where a terminator is to be expected), which is not under your control? \$\endgroup\$ – vnp Feb 22 '15 at 23:13
  • \$\begingroup\$ It will be NULL, because static in most modern compilers ensures the buddy-memory fragments too. Make a test, but with gcc compiler and with std=c99 \$\endgroup\$ – Genis Feb 22 '15 at 23:25
  • \$\begingroup\$ As I said, it will be NULL until somebody else (who claimed that byte) wrote something there. \$\endgroup\$ – vnp Feb 22 '15 at 23:41
  • \$\begingroup\$ Yes.. true that true. \$\endgroup\$ – Genis Feb 22 '15 at 23:45

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.