current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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
share|improve this question
    
I just received an idea for optimization. Make the function understands of endians. But I will leave this to the reviewers. –  Genis Feb 22 at 22:18
2  
You could provide a self-review. –  Hosch250 Feb 22 at 22:22
    
Can I actually do that? –  Genis Feb 22 at 22:34
1  
Yes, you can - especially if it dramatically improves the code. –  Hosch250 Feb 22 at 22:35
    
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). –  Dan Allen Feb 23 at 14:06

1 Answer 1

up vote 3 down vote accepted

  • 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.

share|improve this answer
    
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. –  Genis Feb 22 at 23:02
    
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? –  vnp Feb 22 at 23:13
    
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 –  Genis Feb 22 at 23:25
    
As I said, it will be NULL until somebody else (who claimed that byte) wrote something there. –  vnp Feb 22 at 23:41
    
Yes.. true that true. –  Genis Feb 22 at 23:45

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.