critique: conversion to binary

[bowsayge]

Inspired by fb, Bowsayge decided to write a decimal integer
to binary string converter. Perhaps some of the experienced
C programmers here can critique it. It allocates probably
way too much memory, but it should certainly handle 64-bit
cpus

#include <stdio.h>
#include <stdlib.h>

char * to_binary (unsigned long value) {
const int maxdata = 100;
char * data = malloc(maxdata+1);
int pos = 0;
int pos2 = 0;
if (0 == data) return 0;

if (0 == value) {
return strcpy (data, "0");
}

while (value > 0) {
if (pos >= maxdata) {
printf ("ran out of memory\n");
exit(EXIT_FAILURE);
}
data [pos++] = '0' + (value & 1);
value >>= 1;
}
data[pos--] = 0;

for (pos2 = 0; pos2 < pos; pos2++, pos--) {
char temp = data[pos];
data[pos] = data[pos2];
data[pos2] = temp;
}

return data;
}

int main (int argc, const char * argv [])
{
int ii = 0;
while (ii < argc) {
unsigned long num = ii < 1 ? 5 : atol(argv[ii]);
char * str = to_binary(num);

if (0 == str) return EXIT_FAILURE;
printf ("%lu (decimal) = %s (binary)\n", num, str);
free(str);
ii++;
}
return EXIT_SUCCESS;
}

 

[Turner, GS (Geoff)]

-----Original Message-----
From: bowsayge [mailto:[email protected]]
Posted At: 03 September 2004 09:13
Posted To: c
Conversation: critique: conversion to binary
Subject: critique: conversion to binary


Inspired by fb, Bowsayge decided to write a decimal integer
to binary string converter. Perhaps some of the experienced
C programmers here can critique it. It allocates probably
way too much memory, but it should certainly handle 64-bit
cpus

#include <stdio.h>
#include <stdlib.h>

char * to_binary (unsigned long value) {
const int maxdata = 100;
char * data = malloc(maxdata+1);
int pos = 0;
int pos2 = 0;
if (0 == data) return 0;

if (0 == value) {
return strcpy (data, "0");
}

while (value > 0) {
if (pos >= maxdata) {
printf ("ran out of memory\n");
exit(EXIT_FAILURE);
}
data [pos++] = '0' + (value & 1);
value >>= 1;
}
data[pos--] = 0;

for (pos2 = 0; pos2 < pos; pos2++, pos--) {
char temp = data[pos];
data[pos] = data[pos2];
data[pos2] = temp;
}

return data;
}

int main (int argc, const char * argv [])
{
int ii = 0;
while (ii < argc) {
unsigned long num = ii < 1 ? 5 : atol(argv[ii]);
char * str = to_binary(num);

if (0 == str) return EXIT_FAILURE;
printf ("%lu (decimal) = %s (binary)\n", num, str);
free(str);
ii++;
}
return EXIT_SUCCESS;
}

The following works OK for me.

#include <stdio.h>
#include <string.h>

void Bin2( char *buffer, long n)
{
buffer--;
*buffer = (char) (n%2+'0');
n=n/2;
if (n)
Bin2(buffer,n);
}


int main (void)
{
char strBin[]="00000000"; // or 16, 32 etc
char *fred = strBin+strlen(strBin);

Bin2(fred,22);

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

return 0;
}

GST

 

[Rich Grise]

Inspired by fb, Bowsayge decided to write a decimal integer
to binary string converter. Perhaps some of the experienced
C programmers here can critique it. It allocates probably
way too much memory, but it should certainly handle 64-bit
cpus

#include <stdio.h>
#include <stdlib.h>

char * to_binary (unsigned long value) {
const int maxdata = 100;
char * data = malloc(maxdata+1);
int pos = 0;
int pos2 = 0;
if (0 == data) return 0;

if (0 == value) {
return strcpy (data, "0");
}

while (value > 0) {
if (pos >= maxdata) {
printf ("ran out of memory\n");
exit(EXIT_FAILURE);
}
data [pos++] = '0' + (value & 1);
value >>= 1;
}
data[pos--] = 0;

for (pos2 = 0; pos2 < pos; pos2++, pos--) {
char temp = data[pos];
data[pos] = data[pos2];
data[pos2] = temp;
}

return data;
}

int main (int argc, const char * argv [])
{
int ii = 0;
while (ii < argc) {
unsigned long num = ii < 1 ? 5 : atol(argv[ii]);
char * str = to_binary(num);

if (0 == str) return EXIT_FAILURE;
printf ("%lu (decimal) = %s (binary)\n", num, str);
free(str);
ii++;
}
return EXIT_SUCCESS;
}

Do you see the memory leak?

It's also an awful lot of rigamarole - why not just allocate temp[] to write
the bit digits to, then do data[pos++] = temp [pos2--]. You could also stack
them.

Cheers!
Rich

 

[Does It Matter]

Inspired by fb, Bowsayge decided to write a decimal integer
to binary string converter. Perhaps some of the experienced
C programmers here can critique it. It allocates probably
way too much memory, but it should certainly handle 64-bit
cpus

Gak! K.I.S.S.

The code below is unnecessarily complex. Keep it simple. The fact that
something this simple takes more than 25 lines to do makes it harder to
understand and maintain. A quick glance at this code and I don't
immediately know what you are attempting to do.

Additionally, allocate the amount of memory you need. Using <limits.h> you
should be able to figure out how many bits (char in the array) you need to
represent an unsigned long integer.

#include <stdio.h>
#include <stdlib.h>

char * to_binary (unsigned long value) {
const int maxdata = 100;

Try sizeof(value)*CHAR_BIT/3+1. This will be a little too many bytes but
it is safer than a magic number like 100. What happens if this code goes
into a 64-bit machine and years later gets recompiled on a 128 bit
machine? You are only allocating 100 bits. Even if you allocate 512 bits
you are just delaying the problem. I am sure there where people in the
60's program 7 and 8 bit computers who would never imagine there would be
64 bit computers. So they hard coded things for 50 bits.

char * data = malloc(maxdata+1);
int pos = 0;
int pos2 = 0;
if (0 == data) return 0;

Just a style thing, I liked to use NULL == data rather than 0 == data.

if (0 == value) {
return strcpy (data, "0");
}

Why the special case for zero? If you can write the code such that zero is
no different from any other value, do it.

while (value > 0) {
if (pos >= maxdata) {
printf ("ran out of memory\n");
exit(EXIT_FAILURE);
}

It is good that you have some sort of test for exceeding the 100 bit
limit. Unfortunately, you spent the time allocating 100 bytes and
generating 100 bits of data for nothing. It would have been better for the
code to attempt to allocate the correct amount and fail at the malloc.
Less processing.

Additionally, what happened to freeing the memory you allocated? You have
a memory leak.

Also, style issue. I call a utility program and it exits my entire
application. I would not be happy with that. I would prefer this if
statement free the memory and return a NULL pointer.

data [pos++] = '0' + (value & 1);
value >>= 1;
}
data[pos--] = 0;

You appear to be creating the string backwards.

for (pos2 = 0; pos2 < pos; pos2++, pos--) {
char temp = data[pos];
data[pos] = data[pos2];
data[pos2] = temp;
}

And this loop appears to be correcting the mistake. Did you program the
code without this second loop, see the string was backwards and add the
final for loop to correct the problem? I would have corrected the problem
in the first loop.

return data;
}

I created a solution. It is one if statement to see if the malloc failed,
a for loop with one line in the body to create the string, an assignment
after the for loop to end the string and a return statement. Even with
white space, #include statements and the signature it is 18 lines.

int main (int argc, const char * argv [])
{
int ii = 0;
while (ii < argc) {
unsigned long num = ii < 1 ? 5 : atol(argv[ii]);
char * str = to_binary(num);

if (0 == str) return EXIT_FAILURE;
printf ("%lu (decimal) = %s (binary)\n", num, str);
free(str);
ii++;
}
return EXIT_SUCCESS;
}

Hard to see the code is working with this. Converting from hex to binary
is easy. You might want to print out a hex/binary table. Additionally, why
not just use a for loop and print out 0 to 128 then maybe some boundary
cases. You will see an obvious pattern. If the code is wrong the pattern
will be wrong as well.

 

[John Smith]

Just a style thing, I liked to use NULL == data rather than 0 == data.

Just a style thing: I prefer "data == NULL": What's with this fixation on
writing if statements in reverse, just to catch a typo?

 

[Emmanuel Delahaye]

John Smith wrote on 03/09/04 :

Just a style thing: I prefer "data == NULL": What's with this fixation on
writing if statements in reverse, just to catch a typo?

Yes. I'm not found of it, but it can help sometimes.

--
Emmanuel
The C-FAQ: http://www.eskimo.com/~scs/C-faq/faq.html
The C-library: http://www.dinkumware.com/refxc.html

"C is a sharp tool"

 

[CBFalconer]

Just a style thing: I prefer "data == NULL": What's with this fixation on
writing if statements in reverse, just to catch a typo?

Just to catch a typo

 

[bowsayge]

Turner, GS (Geoff) said to us:

The following works OK for me.

void Bin2( char *buffer, long n)

[...]

Yours is a lot more compact. Thanks.

 

[bowsayge]

Rich Grise said to us:

while (ii < argc) {
unsigned long num = ii < 1 ? 5 : atol(argv[ii]);
char * str = to_binary(num);

if (0 == str) return EXIT_FAILURE;
printf ("%lu (decimal) = %s (binary)\n", num, str);
free(str);
ii++;
}

Do you see the memory leak?

No, the "free (str)" above releases the memory.

It's also an awful lot of rigamarole - why not just allocate temp[] to
write the bit digits to, then do data[pos++] = temp [pos2--].
You could also stack them.

Yes! That'll eliminate the need to reverse the string. Thanks.

 

[CBFalconer]

Rich Grise said to us:
.... snip ...

It's also an awful lot of rigamarole - why not just allocate
temp[] to write the bit digits to, then do
data[pos++] = temp[pos2--]. You could also stack them.

Yes! That'll eliminate the need to reverse the string. Thanks.

Nothing wrong with reversing strings. A routine to do so is short
and quite efficient, and useful elsewhere. Now that you have
beaten this to death take a look at the following (which I have
published before). Only the first 80 odd lines are generically
useful, the rest is stuff to exercise it and convince oneself that
it works correctly.

/* Routines to display values in various bases */
/* with some useful helper routines. */
/* by C.B. Falconer, 19 Sept. 2001 */
/* Released to public domain. Attribution appreciated */

#include <stdio.h>
#include <string.h>
#include <limits.h> /* ULONG_MAX etc. */

/* ======================= */
/* reverse string in place */
size_t revstring(char *stg)
{
char *last, temp;
size_t lgh;

lgh = strlen(stg);
if (lgh > 1) {
last = stg + lgh; /* points to '\0' */
while (last-- > stg) {
temp = *stg; *stg++ = *last; *last = temp;
}
}
return lgh;
} /* revstring */

/* ============================================ */
/* Mask and convert digit to hex representation */
/* Output range is 0..9 and a..f only */
int hexify(unsigned int value)
{
static char hexchars[] = "0123456789abcdef";

return (hexchars[value & 0xf]);
} /* hexify */

/* ================================================== */
/* convert unsigned number to string in various bases */
/* 2 <= base <= 16, controlled by hexify() */
/* Returns actual output string length */
size_t basedisplay(unsigned long number, unsigned int base,
char *stg, size_t maxlgh)
{
char *s;

/* assert (stg[maxlgh]) is valid storage */
s = stg;
if (maxlgh && base)
do {
*s = hexify(number % base);
s++;
} while (--maxlgh && (number = number / base) );
*s = '\0';
revstring(stg);
return (s - stg);
} /* basedisplay */

/* ================================================ */
/* convert signed number to string in various bases */
/* 2 <= base <= 16, controlled by hexify() */
/* Returns actual output string length */
size_t signbasedisplay(long number, unsigned int base,
char * stg, size_t maxlgh)
{
char *s;
size_t lgh;
unsigned long n;

s = stg; lgh = 0;
n = (unsigned long)number;
if (maxlgh && (number < 0L)) {
*s++ = '-';
maxlgh--;
n = -(unsigned long)number;
lgh = 1;
}
lgh = lgh + basedisplay(n, base, s, maxlgh);
return lgh;
} /* signbaseddisplay */


/* ==================== */
/* flush to end-of-line */
int flushln(FILE *f)
{
int ch;

while ('\n' != (ch = fgetc(f)) && (EOF != ch)) /* more */;
return ch;
} /* flushln */

/* ========== END of generically useful routines ============ */

/* ========================= */
/* Prompt and await <return> */
static void nexttest(char *prompt)
{
static char empty[] = "";

if (NULL == prompt) prompt = empty;
printf("\nHit return for next test: %s", prompt);
fflush(stdout);
flushln(stdin);
} /* nexttest */

/* ============================== */
/* Display a value and its length */
static void show(char *caption, int sz, char *stg)
{

if ((unsigned)sz != strlen(stg))
printf("Something is wrong with the sz value\n");
printf("%s: sz = %2d \"%s\"\n", caption, sz, stg);
} /* show */

/* =========== */
/* exercise it */
int main(void)
{
#define LGH 40
#define VALUE 1234567890

char stg[LGH];
unsigned int base;
int sz;

printf("\nExercising basedisplay routine\n");
printf("\nbase sz value\n");
for (base = 2; base <= 16; base++) {
sz = (int)basedisplay(VALUE, base, stg, LGH - 1);
printf("%2d %2d %s\n", base, sz, stg);
}

nexttest("ULONG_MAX");
for (base = 8; base <= 16; base++) {
sz = (int)basedisplay(ULONG_MAX, base, stg, LGH - 1);
printf("%2d %2d %s\n", base, sz, stg);
}

basedisplay(0, 10, stg, 3);
printf("\nzero %s\n", stg);

basedisplay(VALUE, 10, stg, 3);
printf("3 lsdigits only, base 10 %s\n", stg);

printf("\nBad calls:\n");

sz = (int)basedisplay(VALUE, 10, stg, 0);
show("0 length field", sz, stg);

sz = (int)basedisplay(VALUE, 1, stg, 20);
show("base 1, lgh 20", sz, stg);

sz = (int)basedisplay(VALUE, 0, stg, 20);
show("base 0, lgh 20", sz, stg);

sz = (int)signbasedisplay(-1234, 10, stg, 0);
show("0 lgh fld, -ve", sz, stg);

sz = (int)signbasedisplay(-1234, 10, stg, 2);
show("truncate -1234", sz, stg);

nexttest("System limits");

sz = (int)signbasedisplay(SCHAR_MIN, 10, stg, 20);
show("SCHAR_MIN ", sz, stg);

sz = (int)signbasedisplay(SCHAR_MAX, 10, stg, 20);
show("SCHAR_MAX ", sz, stg);

sz = (int)signbasedisplay(UCHAR_MAX, 10, stg, 20);
show("UCHAR_MAX ", sz, stg);

sz = (int)signbasedisplay(CHAR_MIN, 10, stg, 20);
show("CHAR_MIN ", sz, stg);

sz = (int)signbasedisplay(CHAR_MAX, 10, stg, 20);
show("CHAR_MAX ", sz, stg);

sz = (int)signbasedisplay(MB_LEN_MAX, 10, stg, 20);
show("MB_LEN_MAX ", sz, stg);

sz = (int)signbasedisplay(SHRT_MIN, 10, stg, 20);
show("SHRT_MIN ", sz, stg);

sz = (int)signbasedisplay(SHRT_MAX, 10, stg, 20);
show("SHRT_MAX ", sz, stg);

sz = (int)signbasedisplay(USHRT_MAX, 10, stg, 20);
show("USHRT_MAX ", sz, stg);

sz = (int)signbasedisplay(INT_MIN, 10, stg, 20);
show("INT_MIN ", sz, stg);

sz = (int)signbasedisplay(INT_MAX, 10, stg, 20);
show("INT_MAX ", sz, stg);

sz = (int)signbasedisplay(INT_MAX, 10, stg, 20);
show("INT_MAX ", sz, stg);

sz = (int) basedisplay(UINT_MAX, 10, stg, 20);
show("UINT_MAX ", sz, stg);

sz = (int)signbasedisplay(LONG_MIN, 10, stg, 20);
show("LONG_MIN ", sz, stg);

sz = (int)signbasedisplay(LONG_MAX, 10, stg, 20);
show("LONG_MAX ", sz, stg);

sz = (int) basedisplay(ULONG_MAX, 10, stg, 20);
show("ULONG_MAX ", sz, stg);

nexttest("DONE");
return 0;
} /* main */