How can this integer-to-hex string code be improved?

Question

I am using this as a learning exercise primarily and want to ensure my code is optimal. The main thing is that it is reliable and I am made aware of any flaws.

Could it be made more efficient? More readable? Any feedback gladly appreciated. How about style?

I thought about returning a char*, but then the caller would have to think to deallocate the returned string. I saw this as a problem, so I left to caller to allocate and deallocate. Is that the right decision? Any comments?

/* Generate hex string from integer.  Odd number of characters must be preceded 
   with zero character
   Eg. 9 becomes "09", 10 becomes "0A" and 16 becomes "10"
*/

#include <stdio.h>

unsigned int num_hex_digits(unsigned int n) {
    int ret = 0;
    while(n) {
        n >>= 4;
        ++ret;
    }
    return ret;
}

void make_hex_string_easy(unsigned int invokeid, char** xref) 
{ 
    int pad = 0;
    int len = num_hex_digits(invokeid);
    /* if odd number, add 1 - zero pad number */
    if(len % 2 != 0)
        pad = 1;

    sprintf(*xref, "%s%X", pad ? "0" : "", invokeid);
}

void make_hex_string_learning(unsigned int invokeid, char** xref) 
{                                                                                                                                                                                                                               
    char* p = *xref;
    int pad = 0;
    int len = num_hex_digits(invokeid);
    /* if odd number, add 1 - zero pad number */
    if(len % 2 != 0)
        pad = 1;

    /* move to end of number string */
    p+= len + pad - 1;

    while(invokeid) {
        int tmp = invokeid & 0xF;
        if(tmp < 10)
            *p = tmp + '0';
        else 
            *p = tmp + 'A' - 10;

        invokeid >>= 4;

        p--;    
    } 

    if(pad) {
        *p = '0';
    }
}


int main() {
    unsigned int testdata[] = {~0, 1, 255, 256, 0xFFFE, 0xFFFF, 0x10000, 0xABC };
    int sz = sizeof(testdata) / sizeof(int);
    int i;

    char* test = (char*) calloc (20, 1);
    printf("Using sprintf method\n");
    for(i = 0; i < sz; ++i) {
        make_hex_string_easy(testdata[i], &test);
        printf("hex string of %#10x = \t%10s\n", testdata[i], test);
        memset(test, 0, 20);
    }

    printf("\nUsing homegrown method\n");
    for(i = 0; i < sz; ++i) {
        make_hex_string_learning(testdata[i], &test);
        printf("hex string of %#10x = \t%10s\n", testdata[i], test);
        memset(test, 0, 20);
    }
    free(test);

    return 0;
}

Output on my PC:

Using sprintf method
hex string of 0xffffffff =        FFFFFFFF
hex string of        0x1 =              01
hex string of       0xff =              FF
hex string of      0x100 =            0100
hex string of     0xfffe =            FFFE
hex string of     0xffff =            FFFF
hex string of    0x10000 =          010000
hex string of      0xabc =            0ABC

Using homegrown method
hex string of 0xffffffff =        FFFFFFFF
hex string of        0x1 =              01
hex string of       0xff =              FF
hex string of      0x100 =            0100
hex string of     0xfffe =            FFFE
hex string of     0xffff =            FFFF
hex string of    0x10000 =          010000
hex string of      0xabc =            0ABC

UPDATE.

I can't thank you all enough for the brilliant feedback. Choosing an answer was tough and I chose William Morris' because of the zero and hex lookup idea which I like.

Here is my update which could probably go through more cycles. I left in some old, less optimal code to remind me of my iterations. Maybe constructive for others to view too.

/* Generate hex string from integer.  Odd number of characters must be preceded 
   with zero character
   Eg. 9 becomes "09", 10 becomes "0A" and 16 becomes "10"
*/

#include <stdio.h>
#include <math.h>

unsigned num_hex_digits_simple(unsigned n) {
    int ret = 0;
    while(n) {
        n >>= 4;
        ++ret;
    }
    return ret;
}

/* should be faster than simple form above */
static unsigned num_hex_digits(unsigned n) {
    return n ? floor(log(n) / log(16.0)) + 1 : 1;
}

unsigned make_hex_string_learning_ugly(unsigned n, char* s) 
{                                                                                                                                                                                                                               
    char* p = s;
    int pad = 0;
    int len = num_hex_digits(n);

    /* if odd number, add 1 - zero pad number */
    if(len % 2)
        pad = 1;

    /* caller may wish to determine how much to allocate for string and pass null */
    if(s) {
        /* move to end of number string */
        p+= len + pad - 1;

        if(n == 0) {
            *p-- = '0';
        }
        else {
            while(n) {
                int tmp = n & 0xF;
                *p = tmp + (tmp < 10 ? '0' : 'A' - 10);
                n >>= 4;
                p--;    
            }
        }

        if(pad)
            *p = '0';
    }

    return len + pad;
}

unsigned make_hex_string_learning(unsigned n, char *s)
{
    const char hex_lookup[] = "0123456789ABCDEF";
    int len = num_hex_digits(n);
    int i = len;

    if(s) {
        if (i & 1) {
            *s++ = '0';
        }
        s[i] = '\0';

        for (--i; i >= 0; n >>= 4, --i) {
            s[i] = hex_lookup[n & 0xf];
        }
    }
    return len + (len & 1);
}

int main() {
    unsigned testdata[] = {0, ~0, 1, 255, 256, 0xFFFE, 0xFFFF, 0x10000, 0xABC };
    int sz = sizeof(testdata) / sizeof(unsigned);
    int i;
    char test[50] = {0}; 

    printf("Using sprintf method\n");
    for(i = 0; i < sz; ++i) {
        sprintf(test, "%s%X", num_hex_digits(testdata[i]) % 2 ? "0" : "", testdata[i]);
        printf("hex string of %#10x = \t%10s\n", testdata[i], test);
        memset(test, 0, sizeof(test));
    }

    printf("\nUsing homegrown method\n");
    for(i = 0; i < sz; ++i) {
        make_hex_string_learning(testdata[i], test);
        printf("hex string of %#10x = \t%10s\n", testdata[i], test);
        memset(test, 0, sizeof(test));
    }

    return 0;
}

I'd change the name invokeid to something more clear and relevant to its role in the function. I'll write an actual answer in a few minutes. — busy_wait, Aug 31 '13 at 18:03

William Morris · Accepted Answer · 2013-09-01 01:22:39Z

There are two errors in your code and some aspects that are not so nice.

The errors first.

you don't handle 0
you don't terminate the string, instead relying on the buffer having been cleared.

The task is a little strange in requiring a leading zero is the length is odd. A leading zero is often used to show that a number is octal (base 8). If it were for real instead of practice I'd try to drop that requirement.

To fix the non-handling of zero, you need to fix num_hex_digits:

static int num_hex_digits(unsigned n)
{
    if (!n) return 1;

    int ret = 0;
    for (; n; n >>= 4) {
        ++ret;
    }
    return ret;
}

Here I added an explicit check for zero and reorganised the loop into a for loop, which is more normal (it keeps the test of the number and its shifting in the same place). Note also that the function is static because it is a support function for use only here.

Your make_hex_string_learning has some issues. Others have already noted that you only need a simple pointer, not a double pointer. And the parameter names are too long/strange. You used n in the previous function so why not be consistent. And the string can simply be s.

Your use of pad at each end of the function is unnecessary. It is used to adjust the string length and add the leading '0'. Both can be done at the start of the function.

Here is a simplified version of the function:

void make_hex_string_learning(unsigned n, char *s)
{
    const char hex_lookup[] = "0123456789abcdef";
    int len = num_hex_digits(n);

    if (len & 1) {
        *s++ = '0';
    }
    s[len] = '\0';

    for (--len; len >= 0; n >>= 4, --len) {
        s[len] = hex_lookup[n & 0xf];
    }
}

Notice that I added the leading 0 at the start of the function, that I used a simple lookup table to get the character values, that I terminated the string and that I used len as an index instead of advancing the pointer to the end of the string and regressing. I also changed your while loop to a for for the same reason as above.

Aseem Bansal · Answer 2 · 2013-08-31 18:23:19Z

up vote 2 down vote

Don't typecast return of malloc in C. Same thing goes for calloc
invokeid seems to me two words so why no underscore? Be consistent even if it seems obvious.
if(len % 2 != 0) can be replaced by if(len % 2) for efficiency. Eliminates the comparison without any difference in functionality.
You can eliminate len by using the (num_hex_digits(invokeid) % 2) instead of (len % 2). It serves no other purpose and the name of the function is more descriptive here than len anyways
Depending upon the length of the integers that you want to change using log base 2 should be faster to find the length of the integer.You'll have to time this one.
In the function make_hex_string_learning using the ternary operator should be better for readability when assigning to *p.
You are inconsistent in your use of braces around single statements in the make_hex_string_learning function. You shouldn't be.
Your comment /* if odd number, add 1 - zero pad number */ is misleading. You are assigning to pad. I don't think the comment will help anyone understand the code.
if(tmp < 10) Why 10? Why not 20 or 30? Magic numbers are bad. You should avoid them when you can. Add a comment if it isn't possible to avoid them. Otherwise you may end end up asking this same question yourself later on.
You need to add comments in some of your functions. It isn't clear what the purpose of function such as make_hex_string_easy is. What does easy mean here? Not detailed explanation but the expected outputs.
Any reason that you are using extra newlines in your code? It doesn't make reading the code any easier. A single newline between function definitions is justified. It is justified when separating logical blocks but at random places it is just making more scrolling necessary.

edited Aug 31 '13 at 18:23

answered Aug 31 '13 at 18:04

Aseem Bansal
1,3381931

2

I disagree with most of this. Adding a variable to improve readability is great practice and most of the time the compiler will optimize it away anyway. I'd say pad should stay. – busy_wait Aug 31 '13 at 18:07

2

The if is more readable IMO with the comparison. It makes no difference at runtime, but being explicit is better unless the condition is really complicated (and then you have a different problem anyway). – busy_wait Aug 31 '13 at 18:09

IMO when assigning to a single variable a ternary operator is better. Yeah it makes no difference at runtime. I never implied that. Using ternary operator is by no means implicit. It is well defined syntax. – Aseem Bansal Aug 31 '13 at 18:16

@busy_wait Bad call on pad but having len serves nothing. The function is readable as such. No? – Aseem Bansal Aug 31 '13 at 18:20

I'm just saying the original sprintf line is far more readable to me than your suggestion. Yours took me a few seconds to parse. – busy_wait Aug 31 '13 at 18:21

| show 3 more comments

Loki Astari · Answer 3 · 2013-08-31 18:15:05Z

Nothing jumps out:

Rather than this:

   if(tmp < 10)
        *p = tmp + '0';
    else 
        *p = tmp + 'A' - 10;

I would have used:

   *p = tmp + (tmp < 10) ? '0' : 'A' - 10;

Even though you pass around a pointer to a pointer (for re-alloc I assume). You don't actually do any reallocation. So I would simplify the code and just pass a pointer.

void make_hex_string_easy(unsigned int invokeid, char** xref) 
                                                 ^^^^^^   I would just use char*

Note: A lot of common interfaces when you pass a NULL as the destination return how many characters it would have used in the buffer. This leads to a lot of C code that looks like this:

size_t  s = make_hex_string_easy(number, NULL); // assuming you changed your code to return a value.
char*   b = malloc(s+1);
make_hex_string_easy(number, b);

But Since you don't use dynamic buffers I would remove the calloc.

printf("Using sprintf method\n");
for(i = 0; i < sz; ++i) {
    char  test[50] = {0}; // inits to 0
    make_hex_string_easy(testdata[i], test);  // assuming you change to just a pointer
    printf("hex string of %#10x = \t%10s\n", testdata[i], test);
}

asked	2 years ago
viewed	2804 times
active	2 years ago

current community

your communities

more stack exchange communities

How can this integer-to-hex string code be improved?

3 Answers 3

Your Answer

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

Hot Network Questions

current community

your communities

more stack exchange communities

How can this integer-to-hex string code be improved?

3 Answers 3

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions