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.

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;
}
share|improve this question
2  
I see that you have edited in your revised code into your question. That is fine, just understand that your revised code is unlikely to get a review, and that it would be better to post it as a new question. See this meta post for more information. –  syb0rg Mar 3 at 22:48

3 Answers 3

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.

share|improve this answer

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.

share|improve this answer
    
Right, thanks. I fixed the comment to say empty string. Going backwards was just a choice. I guess it could go the other way too. –  user37718 Mar 2 at 22:58
    
Also, the unsigned parameter was used so that an arithmetic shift would not happen. That may be a bad assumption, so I could change the direction in which the string is processed and do a left shift and make the parameters consistent. –  user37718 Mar 2 at 23:03

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.

share|improve this answer
    
Doesn't "little-endian" apply to the way in which the bytes within integers are arranged? If so, how does that affect strings, or affect the OP's string-building or string-parsing functions? –  ChrisW Mar 3 at 11:48
    
There are cases where it applies to both byte order and bit order in bytes. So I used the term. Nevertheless I am wrong, because the endianess is ok in Steves code, and I will remove the part in my answer. - But the rest still applies, a function called str2Int returning a long is the main problem. –  Marcel Blanck Mar 3 at 12:28
    
I agree on the for loop, but I was going through K&R 2nd and saw similar usage in section 2.1. –  user37718 Mar 4 at 2:11
    
Hi Steve, I think while K&R might be nice for learning, it's not so great for production code. See my explanation above in the note to the answer. –  Marcel Blanck Mar 20 at 19:57

Your Answer

 
discard

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