5
\$\begingroup\$

To practice my C, I've written a small function in C to display an integer as an 8-bit binary number. Please let me know how to improve this function, and other C-style coding conventions. Thanks!

/**
 * Converts an integer to binary.
 * 
 * @param num Integer to convert.
 * 
 * @return c-string.
**/
const char* to_binary(int num) {

    static char binary[10];
    binary[0] = '0';
    binary[1] = 'b';

    int value = 128;
    int number = num;

    for (int i = 2; i < 10; i++) {
        if (number - value >= 0) {
            binary[i] = '1';
            number = number - value;
        } else {
            binary[i] = '0';
        }
        value = value / 2;
    }

    return binary;

}

And here's how you call it:

int main() {

    const char* result = to_binary(146);

    printf("%s\n", result);

}
\$\endgroup\$
10
\$\begingroup\$
  • More specific types are available for an eight bit number, such as uint8_t
  • static keyword is dangerous (when used as an output buffer) and results in weird behaviour if your method is called twice
  • The arithmetic can be replaced with a more simple & (bitwise AND)
void to_binary(uint8_t x, char *output) {
    *output++ = '0';
    *output++ = 'b';
    for (int mask = 1<<CHAR_BIT-1; mask!=0 ; mask>>=1) {
        *output++ = x & mask ? '1':'0'; 
    }
    *output = 0; // null terminate the string
}
\$\endgroup\$
12
  • 6
    \$\begingroup\$ static isn't weird and dangerous. It means the data is allocated in either .DATA or the .BBS section of the binary, instead of the stack. This means a variable is allocated once, and the value "kept" between calls. It has legitimate uses. \$\endgroup\$ – AnnoyinC Jan 21 at 10:07
  • 9
    \$\begingroup\$ @AnnoyinC, I think Ted's point is that using static storage makes the function dangerous and weird. Users don't expect such a function to be stateful like that. BTW, C standard says nothing about "segments" - that's a platform-specific mechanism. \$\endgroup\$ – Toby Speight Jan 21 at 12:05
  • 2
    \$\begingroup\$ 2<<i should be 1<<i and you need to start from the MSB, for (int i = total_bits - 1; i >= 0 ; --i). \$\endgroup\$ – Stephen Kitt Jan 21 at 16:31
  • 2
    \$\begingroup\$ Instead of counting with i and generating the mask on the fly, you can do something like for (mask = 1 << 7; mask != 0; mask >>=1), this is naively more efficient, but the compiler probably does this already at O1 or so. \$\endgroup\$ – 12345ieee Jan 21 at 17:55
  • 2
    \$\begingroup\$ Your placement of spaces in the code is strange. \$\endgroup\$ – Roland Illig Jan 22 at 1:07
7
\$\begingroup\$

Bug

Code attempts to form a string. Yet it lacks a terminating null character and space for it.

// static char binary[10];
static char binary[11];

As a static, it is initialized to all zeros. With increased size, an explicit setting of the null character is not needed.

OP's code undefined behavior perhaps "works" as a zero may exist just past binary[10].

Loop Simplification

Iterate right (most significant) to left (least significant).

for (int i = 10; --i >= 2; ) {
  binary[i] = '0' + (num & 1);
  num /= 2;
}

Unclear functionality

When num > 255 code prints all '1'.
When num < 0 code prints all '0'.

Code should 1) not use an int argument or 2) state functionality when num is outside the [0-255] range.

Alternative

I'd avoid the use of static and provide a buffer to the function. Example.

\$\endgroup\$
6
  • \$\begingroup\$ Just a thought, but it might work with 10 because there could be unused bytes after for alignment. \$\endgroup\$ – Ben Jan 21 at 22:02
  • \$\begingroup\$ @ben Agree. See "OP's code undefined behavior perhaps "works" as a zero may exist just past binary[10]." \$\endgroup\$ – chux - Reinstate Monica Jan 21 at 23:11
  • \$\begingroup\$ @Ben even if there are "unused bytes for alignment", there is no guarantee that these bytes are initialized to 0. Still undefined behavior. \$\endgroup\$ – Roland Illig Jan 22 at 1:05
  • \$\begingroup\$ @RolandIllig, hence the might. \$\endgroup\$ – Ben Jan 22 at 13:05
  • \$\begingroup\$ @L.F. Agree static char are zero-initialized. Yet the bytes near, though not part of, binary[10] are the issue. They are not specified to be zero nor even accessible. \$\endgroup\$ – chux - Reinstate Monica Jan 22 at 14:15

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.