Integer-to-hex string generator

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 or more readable? 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

Answer 1

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.

Answer 2

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);
}

Answer 3

• 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.

Answer 4

Use variable width format specifier

When using sprintf, you can use a variable width specifier, and also a "pad with zero" specifier, so that you don't need to do these things manually:

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

    sprintf(*xref, "%0*X", len, invokeid);
}

Here, after the % character, the 0 tells sprintf to pad the hex value with zeros. For example:

printf("%08x", 0x1234);  // Will print "00001234" due to the %08x specifier

After the 0 there is a * character. This tells sprintf that the width will come from a variable instead of being hardcoded. For example:

printf("%0*x", 6, 0x1234); // Will print "001234" with a width of 6

So putting it all together, "%0*X" prints a zero padded hex string whose width comes from the next argument. In the modified code above, that argument is len.