0
\$\begingroup\$

Here is a basic itoa function that I've written trying to use recursion:

void itoa(int number, char buffer[])
{
    static int idx=0, size=0;
    if (number == 0) {
        buffer[size] = '\0';
        idx = size = 0; // reset for next call
    }
    else {
        size++, idx++;
        itoa(number/10, buffer);
        buffer[size-idx--] = (number % 10) + '0';
    }
}

How does it look? It is not thread-safe due to the static, but is that an issue for a function such as this? If so, how could I update that? What other ways could I make improvements to it?

\$\endgroup\$
7
  • 4
    \$\begingroup\$ How does it look? - scary. Besides, if number is 0 to begin with, it doesn't give the correct result. \$\endgroup\$
    – vnp
    Mar 19 at 7:23
  • \$\begingroup\$ Why did you want a recursive function? That's usually a poor choice in C if there's a simple iterative version possible. \$\endgroup\$ Mar 19 at 11:49
  • \$\begingroup\$ How do you know the buffer is big enough? The recursion doesn't seem to place things in the right place... Couldn't you just use sprintf? \$\endgroup\$
    – Mr R
    Mar 19 at 14:33
  • \$\begingroup\$ @MrR 1. It's up to caller to provide the buffer long enough. 3. I suppose the printf functions family utilizes the same routine for int arguments as itoa() does, so redirecting itoa to sprintf may be a kind of an infinite loop.... \$\endgroup\$
    – CiaPan
    Mar 19 at 14:55
  • 1
    \$\begingroup\$ Your code doesn't seem to care about negative values of number... \$\endgroup\$
    – CiaPan
    Mar 19 at 15:01
2
\$\begingroup\$

How does it look?

Not so good.

  1. Not thread safe.

  2. Does not handle negative numbers well.

  3. Forms "" with 0.

  4. Prone to buffer overflow.

  5. itoa() is not a standard function, yet it commonly returns a pointer.

A test harness

int main(void) {
  char buf[100];
  int test[] = {123, 456, 0, - 42, INT_MAX, INT_MIN};
  int n = sizeof test / sizeof *test;
  for (int i = 0; i < n; i++) {
    itoa(test[i], buf);
    printf("%-11d <%s>\n", test[i], buf);
  }
  return 0;
}

Output

123         <123>
456         <456>
0           <>
-42         <,.>
2147483647  <2147483647>
-2147483648 <./,),(-*,(>

It is not thread-safe due to the static, but is that an issue for a function such as this?

Yes. Thread safety is expected.


If so, how could I update that? What other ways could I make improvements to it?

I really do not think this is a good place to use recursion given the potential for buffer overflow is a fair complication.

But if one must use recursion, consider adding error checking and test for various sorts of int including 0, INT_MAX, INT_MIN.

static char* itoa_helper(int number, size_t sz, char buffer[]) {
  if (sz == 0) {
    return NULL;
  }
  if (number <= -10) {
    buffer = itoa_helper(number / 10, sz - 1, buffer);
    if (buffer == NULL) {
      return NULL;
    }
    number %= 10;
  }
  *buffer = (char) ('0' - number);
  return buffer + 1;
}

char* itoa_recursive_alt(int number, size_t sz, char buffer[sz]) {
  if (sz == 0 || buffer == NULL) {
    return NULL;
  }
  char *s = buffer;

  if (number >= 0) {
    // Flip pos numbers to neg as neg range is greater.
    number = -number;
  } else {
    sz--;
    if (sz == 0) {
      *buffer = '\0';
      return NULL;
    }
    *s++ = '-';
  }
  s = itoa_helper(number, sz-1, s);
  if (s == NULL) {
    *buffer = '\0';
    return NULL;
  }
  *s = 0;
  return buffer;
}

int main(void) {
  char buf[100];
  int test[] = {123, 456, 0, -42, INT_MAX, INT_MIN};
  int n = sizeof test / sizeof *test;
  for (int i = 0; i < n; i++) {
    // char *s = itoa_recursive_alt(test[i], sizeof buf, buf);
    char *s = itoa_recursive_alt(test[i], sizeof buf, buf);
    if (s == NULL)
      s = "NULL";
    printf("%-11d <%s> <%s>\n", test[i], s, buf);
  }
  return 0;
}

Output

123         <123> <123>
456         <456> <456>
0           <0> <0>
-42         <-42> <-42>
2147483647  <2147483647> <2147483647>
-2147483648 <-2147483648> <-2147483648>
\$\endgroup\$
5
  • \$\begingroup\$ awesome, thank you so much for putting in the time. A few questions: (1) what's the downside of not doing the covert-to-negative? (2) what's the downside of not passing a size to the itoa function? Can't the callee determine the size and just set a max-length and return an error if there's truncation? \$\endgroup\$
    – carl.hiass
    Mar 20 at 3:38
  • 1
    \$\begingroup\$ @carl.hiass 1) what's the downside of not doing the covert-to-negative? --> Explain how you are going to handle INT_MIN which is less than -INT_MAX. 2) what's the downside of not passing a size to the itoa function? --> Risking buffer overflow 3) Can't the callee determine the size and just set a max-length and return an error if there's truncation? --->Unless the caller provides a size, the callee does not know how much is too much. The callee does not control the amount of buffer that can be used. The caller knows though. \$\endgroup\$ Mar 20 at 3:45
  • \$\begingroup\$ @carl.hiass Was recursion a requirement? \$\endgroup\$ Mar 20 at 3:46
  • \$\begingroup\$ no I was just learning about it and trying out with that. I thought it might get rid of the need to reverse (which it does) but obviously there are a lot of other issues it introduces. \$\endgroup\$
    – carl.hiass
    Mar 20 at 4:43
  • \$\begingroup\$ I've written an updated version based on your help -- thanks again! codereview.stackexchange.com/questions/257428/… \$\endgroup\$
    – carl.hiass
    Mar 20 at 4:51

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.