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

The style is good enough, but there are some pretty glaring logic errors. Besides that, the coding just seems sort of mediocre or uninspired in general, honestly. I would suggest a bit more consideration of potential issues of malformed input, avoiding global values wherever possible, returning values in place of using asserts (the latter are really for debug mode builds to pinpoint logic errors anyway, consider setting errno instead). Also, the use of static in the main file looks a bit amateurish, as do the solo increment statements and hard-coded format strings. You might save yourself some clock cycles and real estate by calculating the result length apriori, too (the log function works well for that). So just tighten things up a bit and work on being more robust in the error handling, basically.

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 smokeyangel

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

No, constants are a precisely what asserts were designed for! But you are right that a compile-time check is much more useful. At any rate, the code as it stands is quite literally a segfault waiting to happen. Nevermind that the function is mostly useless, a pointless exercise in K&R style (hey feature writer - how about hexadecimal support?), and arguably the most insipid if aesthetic code I've seen in a long while. But whatever, he doesn't necessarily have to write real world code anyway. Who knows, maybe he could co-author books with Herb Schildt or something?

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.

Originally Posted by Richardcavell

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

Have you even begun to address the BUGS that were pointed out earlier, or are you just too busy patting yourself on the back right now?

Originally Posted by Barney McGrew

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

Okay, so useless to everyone but libc maintainers...

To discover more bugs you should do the printfs like this:

Code:

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

That way notice that the second conversion inserts spaces into the conversion of the number for some reason; it seems incorrect.

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.

Oh, I just noticed the parameter is supposed to adjust the field width. If it's intentional, then fine. But my suggestion to use double quotes around the intended conversion could be used to help see if it has the correct number of spaces.

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!

Originally Posted by smokeyangel

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!

Yes, but constants also come in the form of actual variables, which obviously can't be examined by the preprocessor. Either way, ASSERTS ARE STRICTLY FOR DEBUGGING and should ALWAYS be disabled in the release build, regardless of the size or complexity of the program...

Shoot, mistook an earlier post by Barney McGrew as being one of the OP's and, well, guess I thought he was being complacent and whatnot...my apologies!