Binary string to integer and integer to binary string

Question

I am working through K&R for review and I thought that I would write some of my own routines for conversion of binary strings to int and int to binary string. I realize this is reinvetning the wheel; I am just doing this for fun. Also, I am attempting to follow this coding style CodingStyle.txt. Any and all critiques are welcomed.

#include <mybin.h>
#include <limits.h>
#include <errno.h>

/*
Parameters:
s   - String with a maximum of 63 binary digits
Return:
    - Decimal value of the binary string pointed to by s
Error:
    - Negative returned on error check errno
*/
signed long binstr2int(char *s)
{
    signed long rc;
    for (rc = 0; '\0' != *s; s++) {
        if (rc > (LONG_MAX/2)) {
            errno = ERANGE;
            return -1;
        } else if ('1' == *s) {
            rc = (rc * 2) + 1;
        } else if ('0' == *s) {
            rc *= 2;
        } else {
            errno = EINVAL;
            return -1;
        }
    }
    return rc;
}

/*
Parameters:
num - The number to convert to a binary string
s   - Pointer to a memory region to return the string to
len - Size in bytes of the region pointed to by s
Return:
    - Pointer to the beginning of the string
Error:
    - NULL returned on error check errno
*/
char *int2binstr(unsigned long num, char *s, int len)
{   
    if (len < 2) {
        errno = ERANGE;
        *s = '\0';
        return s;
    } else if (0 == num) {
        s[--len] = '\0';
        s[--len] = '0';
        return s + len;
    } else { 
        for (s[--len] = '\0'; 0 != num; num >>= 1) {
            if (0 == len) {
                errno = ERANGE;
                *s = '\0';
                return s;
            } else if (num & 1) {
                s[--len] = '1';
            } else {
                s[--len] = '0';
            }
        }
    }
    return s + len;
}

Edit 1: Okay, here is the revised code. Most of the suggestions I agree with. I did retain filling the buffer from end to beginning because it seems like a logical flow to me and requires one less variable to deal with.

• The do while suggestion was spot on. The for loop was unusual and the do while consumed one of the conditionals making the code easier to read.
• The names of the functions have been revised so they are not misleading
• The functions now handle unsigned long ints
• ul2binstr now returns NULL on error. So this needs to be checked.
• Also the string parameter was in binstr2ul was made const
• The len parameter was changed to size_t type

---

/*
Parameters:
s   - String with a maximum of log2(ULONG_MAX) binary characters
num - Memory address to store the result in
Return:
    - Status integer
Error:
    - Negative returned on error check errno
*/
int binstr2ul(const char *s, unsigned long *num)
{
    unsigned long rc;
    for (rc = 0; '\0' != *s; s++) {
        if (rc > (ULONG_MAX/2)) {
            errno = ERANGE;
            return -1;
        } else if ('1' == *s) {
            rc = (rc * 2) + 1;
        } else if ('0' == *s) {
            rc *= 2;
        } else {
            errno = EINVAL;
            return -1;
        }
    }
    *num = rc;
    return 0;
}

/*
Parameters:
num     - The number to convert to a binary string
s       - Pointer to a memory region to return the string to
len     - Size in bytes of the region pointed to by s
Return:
        - Pointer to the beginning of the string
Error:
        - NULL returned on error check errno
*/
char *ul2binstr(unsigned long num, char *s, size_t len)
{
    if (0 == len) {
        errno = EINVAL;
        return NULL;
    } else {
        s[--len] = '\0';
    }

    do {
        if (0 == len) {
            errno = ERANGE;
            return NULL;
        } else {
            s[--len] = ((num & 1) ? '1' : '0');
        }
    } while ((num >>= 1) != 0);

    return s + len;
}

Answer 1

It makes sense for the two functions to be symmetrical - ie that you can call them in turn to swap between integer and string representations. So I would make binstr2int produce an unsigned long of full range returned through a parameter and have the function return the status (0/-1):

int binstr2int(const char *s, unsigned long &num);

Note that s can be const.

In int2binstr I think the characters should be moved to the beginning of the string - this is what all other C string functions do (or at least I'm ignorant of any that do not). Also, your code is a bit awkward in that it treats 0 separately from all other numbers. This is a case where a do {...} while loop makes sense as it can handle the zero case as any other number and test for zero once it has done it:

char* int2binstr(unsigned long num, char *s, size_t size)
{
    long len = (long) size - 1;
    do {
        if (len <= 0) {
            errno = ERANGE;
            return NULL;
        }
        s[--len] = (num & 1) ? '1' : '0';
    } while ((num >>= 1) != 0);

    long n = (long) size - len - 1;
    memmove(s, s + len, n);
    s[n] = '\0';
    return s;
}

Here you can see that the tests for a short input string and for num == 0 are handled naturally. Also I moved the string into place after creating it.

Answer 2

In int2binstr the comment says, "NULL returned on error check errno" but you don't return NULL on error: instead you return a pointer to an empty string.

Also, I find it unusual that int2binstr writes its result at the end of the passed-in buffer, instead of at the beginning.

Also, why use long in the first function but unsigned long in the second function.

Answer 3

I have only taken a quick look at binstr2int...

If you ask me, a function called xxx2int should return an integer. Messed up function names like this cost time, because everybody has to read up how to use it. And the code where it is used becomes less and less literal if naming like this is the overall practice.

Then

signed long rc;
for (rc = 0; '\0' != *s; s++) {

is a tricky use of for, but it is not what a reader expects from a for-loop (the first statement should initiialize something that has to do with the loop conditions) and this may lead to errors if someone refactors or debugs the code. Do not use standard expressions in a tricky way without reason. What you wanted to communicate to a reader was maybe:

signed long rc = 0;
while (0 != *s)
{
    // ...
    s++;
}

If you contribute somewhere or work on a team, the code and all naming has to be contradiction-free and fully literal in usage, this is high priority.

---

An additional Note for Steve in the comments (if this might be allowed one time, because it fit's the topic, but not the comment space):

Hi Steve, personally I do not think K&R is a good reference for production code. And I just have taken a look at chapter 2.1 and have not seen K&R using the third statement for another variable than the one in the first statement. K&R use extreme complex stuff in the middle statement of for-loops in this chapter. Wrapping your head around stuff like this is awesome for learning!

But it is really bad for team owned code. Back in the 70s there was not much thinking about new concepts like code smells, SOLID, KISS, architecture and so on. These concepts grew with programming experience in the community. A clean, understandable, fast to read and trap-free codebase is pure gold for a company and a devs everyday life. That's why my favorite books on this topic are "Refactoring" from Fowler/Beck and "Clean Code" from Martin, while yes - on the unitversity I had also a K&R C Programming Language in my pocket.