7
\$\begingroup\$

How can this solution be improved?

#include <limits.h>

void itoa(int n, char s[])
{
    int min_int = 0;
    int i, sign;
    if (INT_MIN == n) {
        min_int = 1;
        n++;
    }

    if ((sign = n) < 0)
        n = -n;
    i = 0;
    do {
        s[i++] = n % 10 + '0';
    } while ((n /= 10) > 0);

    if (sign < 0)
        s[i++] = '-';

    s[i] = '\0';

    if (min_int)
        s[0]++;

    reverse(s);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ How about just use snprintf()? \$\endgroup\$ – Jeff Mercado Nov 21 '11 at 21:12
  • \$\begingroup\$ @JeffMercado: The only thing that's better with snprintf is that it's standard. Apart from that, it is a horrible slow function that can't be used in application with high real time demands. \$\endgroup\$ – Lundin Nov 22 '11 at 10:34
  • \$\begingroup\$ Funny part. This code is almost exactly Kernighan & Ritchie solution. So it is K&R code review! (source en.wikibooks.org/wiki/C_Programming/C_Reference/stdlib.h/itoa) \$\endgroup\$ – MajesticRa Jul 24 '16 at 19:48
6
\$\begingroup\$

I think I'd break this up into a few separate routines, each with a single more specific intent. I think the easiest way to do this is probably to start by taking the absolute value of the input (represented as an unsigned type), something like this:

unsigned iabs(int input) { 
    if (input >= 0)
        return (unsigned)input;
    return (UINT_MAX - (unsigned)input)+1U;
}

I believe C's requirements for converting a signed to unsigned ensure that this produce correct results for all inputs (including INT_MIN).

With that, nearly everything else can deal with an unsigned number. Personally, I also prefer to generate the result in order instead of generating in reverse, then reversing it. One fairly simple way to do that is with recursion, with each step first generating previous numerals (if any), then adding its own numeral to the end of the string. I'd have the recursive function deal only with an unsigned type (as produced above). Of course, like nearly everything that manipulates data in a buffer in C, you should also pass the maximum buffer length for it to use, to prevent buffer overruns. For the sake of generality, I've also made it accept the base for the conversion as a parameter instead of always assuming base 10.

char *itoa_internal(char *buffer, size_t len, unsigned input, int base) { 
    static const char digits[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    char *pos = buffer;
    if (input >= base)
        pos = itoa_internal(buffer, len, input/base, base);

    if (pos-buffer < len-1)
        *pos++ = digits[input % base];
    return pos;
}

Then I'd have a small wrapper function that puts in a - if the input is negative, invokes the function above with the absolute value of the input, and terminates the string after the internal function returns.

char *itoa(char *buffer, size_t len, int input, int base) {
    char *pos = buffer;

    if (base < 2 || base > 36 || len < 1)
        return NULL;

    if (input < 0)
        *pos++ = '-';

    pos = itoa_internal(pos, len, iabs(input), base);
    *pos = '\0';
    return buffer;
}

This should also make it pretty trivial to write a utoa that handles unsigned inputs -- it would be like itoa without the code to deal with negative numbers. As a minor refinement, I'd probably also make iabs and itoa_internal static functions to minimize the chances of them interfering with other names.

\$\endgroup\$
  • \$\begingroup\$ It is probably bad practice to use unsigned to implicitly refer to unsigned int. If I remember correctly, this is one of the things the C committee will make obsolete in future language standards. The best is of course to use the C99 int types from stdint.h (uint8_t, uint16_t etc) if C99 is available. Also, I don't see why you would use recursion for this, a plain for loop will both be faster and easier to read. \$\endgroup\$ – Lundin Nov 22 '11 at 10:57
  • \$\begingroup\$ @Lundin: Why would it be bad practice? Do you think somebody might think it referred to a nonexistent unsigned float type? I can't imagine the committee changing this -- it would gain nothing, and break huge amounts of code. In this case, the types for stdint.h would be detrimental. I used recursion because I've yet to see an iterative version that was nearly as clean. "Recursion => slow" has been obsolete for a decade or more. \$\endgroup\$ – Jerry Coffin Nov 22 '11 at 11:57
  • \$\begingroup\$ I'm not sure from where I heard that it may become obsolete, but anyway, there's no reason why you can't type out 3 more letters just in case, it isn't that big an effort and makes the code clearer. stdint.h could actually improve the code further - if you know the size of an int, then you can tell whether the [len] parameter is valid or not, if you know that int is 16 bits, then len shouldn't be longer than 5 letters + 1 null. \$\endgroup\$ – Lundin Nov 22 '11 at 12:26
  • \$\begingroup\$ I don't see how recursion could be optimized by the compiler and still C standard-compliant? A function call is a sequence point and all side effects must be complete before the function is called. With all the side effects in place, I don't see how the compiler could unroll the recursion on its own? Also, if it is standard-compliant, mustn't it be tail recursion if the compiler should have any chance at all of optimizing it? I suspect whether the compiler is allowed/able to do this is highly language-dependent. \$\endgroup\$ – Lundin Nov 22 '11 at 12:42
  • \$\begingroup\$ IMO, it does not make the code any clearer. Do you also think there's some improvement from replacing int x; with auto signed int x;? If so, why? If not, why should one use defaults but not the other? Faster recursion is a result of hardware improvements (branch prediction, on-chip return stack, on-chip stack). \$\endgroup\$ – Jerry Coffin Nov 22 '11 at 15:43
5
\$\begingroup\$

Just some general idea about the code (without any effort to create a better algorithm).

First of all, I agree with @Jeff, if there is a library for that use it. You don't have to maintain it and everybody will know what the code does.

So, code. I would use longer variable names:

void itoa(int number, char result[])
int i, sign;

I prefer one declaration per line because it's easier to read and find the type of variables.

if ((sign = n) < 0)

It looks cool but hard to maintain. Just some questions:

  • Why is it here?
  • Is it intentionally = (and not ==)?
  • Why isn't it in a line before?

If you have to maintain this code these kind of question could be hurt. So, I would write

sign = number;
if (number < 0) ...

The questions are the in the while loop:

do { 
    ...
} while ((n /= 10) > 0);

I'd write:

do { 
    ...
    n /= 10;
} while (n > 0);

Finally, I guess there is a potential buffer overflow if the caller pass too small char array to the function.

\$\endgroup\$
  • 2
    \$\begingroup\$ Good comments. There is nothing gained by merging several rows into one, that's just obfuscation and makes the code less readable. \$\endgroup\$ – Lundin Nov 22 '11 at 10:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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