Thread: Please rate at my code

Hi everyone,

I just want to get feedback on this code. I've been studying C seriously for a couple of years now, using K&R. Any comments?

Code:

/* exercise 3-4 from K&R 2

  Write a version of itoa that accepts three arguments instead of two.
  The third argument is a minimum field width; the converted number must
  be padded with blanks on the left if necessary to make it wide enough.

*/

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

#define BASE 10

static char *
reverse(char *s, unsigned int j)
{
  unsigned int i = 0;

  assert (j > 0);
  --j;

  while (i < j)
  {
    char temp = s[i];
    s[i] = s[j];
    s[j] = temp;

    ++i;
    --j;
  }

  return s;
}

static char *
my_itoa(int n, char s[], unsigned int min)
{
  unsigned int negative = (n < 0);
  unsigned int i = 0;
  div_t d;

  assert(BASE >= 2 && BASE <= 10);
  assert(s);

  do
  {
    int digit;
    d = div(n, BASE);
    digit = d.rem;

    if (negative)
    {
      digit = -digit;

      if (digit < 0)
        digit += BASE;
    }

    s[i] = digit + '0';

    assert(s[i] >='0' && s[i] <= (BASE - 1 + '0'));

    n = d.quot;
    ++i;
  }
  while (n != 0);

  if (negative)
  {
    s[i] = '-';
    ++i;
  }

  while (i < min)
  {
    s[i] = ' ';
    ++i;
  }

  reverse(s, i);
  s[i] = 0;

  return s;
}

int main(void)
{
  char s[100];

  printf("my_itoa(12345, s, 5) == \n%s\n", my_itoa(12345, s, 5));
  printf("my_itoa(-67890, s, 10) == \n%s\n", my_itoa(-67890, s, 10));
  printf("my_itoa(-32132145, s, 3) == \n%s\n", my_itoa(-32132145, s, 3));

  return 0;
}

I guess there are two things that pop out at me:

- I think it would be more efficient, in the case that n is negative, if you made n a positive value at the beginning of your code instead of handling that case repeatedly in your loop.
- It doesn't handle bases larger than 10.

You could get around that second issue by defining BASE as a string literal containing a list of digits, where "sizeof BASE - 1" will give you the divisor you need, and "BASE[digit]" will give you the digit.

Aside from those two issues, I'd say it's fine.

All in all I think it looks good.

I agree with Sebastiani about the asserts and error handling. If you have the concept of a "release build" that defines NDEBUG, assert will do nothing. On principle, it's not right to use a "OH no it's ALL GONE WRONG I can't possibly go on!!" piece of functionality in a situation that could easily happen in normal use. In this case, passing s=NULL shouldn't really terminate the program.

One stylistic comment:

Code:

assert(BASE >= 2 && BASE <= 10);

Since BASE is a compile time constant (preprocessor macro) it's not right to have an assert on its value. If BASE is fine, then I expect the compiler will optimise away the assert(true), so no harm done. However if BASE is out of range, you've delayed finding a problem until run time when you could have detected it at compile time. Something like:

Code:

#if BASE < 2 || BASE > 10
#error BASE must be between 2 and 10
#endif

I like that there's very little limiting the bases supported - I think it'll "just work" if you find a way to provide the digits. I like the idea above for that.

Originally Posted by Sebastiani

arguably the most insipid if aesthetic code I've seen in a long while.

Gee, I thought it was pretty good. Thanks for everyone's feedback.

Also, on the subject of changing the number to a positive number and then working on it, that doesn't work if the negative range of int is higher than the positive range - which is certainly true on my system.

Richard

Originally Posted by Richardcavell

that doesn't work if the negative range of int is higher than the positive range - which is certainly true on my system.

Any code using your function ought to use values within the type's minimum guaranteed range, but a good point nonetheless. Have you considered converting it to an unsigned type?

Originally Posted by Sebastiani

Nevermind that the function is mostly useless

It's necessary for the implementation of the *printf family.

Mostly, I'd think. I suppose there are also situations where you might want to print an integer with padding in a base that isn't supported by the *printf family.

Originally Posted by Sebastiani

No, constants are a precisely what asserts were designed for!

Eh what? I can't quite tell if you're joking!!

Asserts are great if you have a big complicated project with plenty of conditions that must hold true at particular points. That's my impression of what they're for: checking conditions which MUST be true, and if they're not true then something completely unexpected has happened -- generally indicating programmer error. I've seen assert(pointer is not NULL) checks in real code (and by seen, I mean screwed up and triggered) where it's truly a disaster to arrive at that point with a NULL pointer. However the most useful asserts during development are the expensive ones -- the ones that try to verify that the world is in a sane state.

So not checking constants, as far as I can tell!