Like Tree2Likes

Please rate at my code

This is a discussion on Please rate at my code within the C Programming forums, part of the General Programming Boards category; Hi everyone, I just want to get feedback on this code. I've been studying C seriously for a couple of ...

  1. #1
    Registered User
    Join Date
    Feb 2011
    Posts
    126

    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;
    }
    jwroblewski44 likes this.

  2. #2
    Guest Sebastiani's Avatar
    Join Date
    Aug 2001
    Location
    Waterloo, Texas
    Posts
    5,600

    Post Needs work...

    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.
    Last edited by Sebastiani; 02-23-2013 at 01:09 AM.
    jwroblewski44 likes this.
    Code:
    if( numeric_limits< byte >::digits != bits_per_byte )
        error( "program requires bits_per_byte-bit bytes" );
    24bbs.cpp

  3. #3
    Stoned Witch Barney McGrew's Avatar
    Join Date
    Oct 2012
    Location
    astaylea
    Posts
    420
    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.

  4. #4
    Registered User
    Join Date
    Mar 2010
    Posts
    531
    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.

  5. #5
    Guest Sebastiani's Avatar
    Join Date
    Aug 2001
    Location
    Waterloo, Texas
    Posts
    5,600

    Arrow

    Quote Originally Posted by smokeyangel View Post
    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?
    Code:
    if( numeric_limits< byte >::digits != bits_per_byte )
        error( "program requires bits_per_byte-bit bytes" );
    24bbs.cpp

  6. #6
    Registered User
    Join Date
    Feb 2011
    Posts
    126
    Quote Originally Posted by Sebastiani View Post
    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

  7. #7
    Stoned Witch Barney McGrew's Avatar
    Join Date
    Oct 2012
    Location
    astaylea
    Posts
    420
    Quote Originally Posted by Richardcavell View Post
    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?

    Quote Originally Posted by Sebastiani
    Nevermind that the function is mostly useless
    It's necessary for the implementation of the *printf family.


  8. #8
    Guest Sebastiani's Avatar
    Join Date
    Aug 2001
    Location
    Waterloo, Texas
    Posts
    5,600
    Quote Originally Posted by Richardcavell View Post
    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?
    Code:
    if( numeric_limits< byte >::digits != bits_per_byte )
        error( "program requires bits_per_byte-bit bytes" );
    24bbs.cpp

  9. #9
    Guest Sebastiani's Avatar
    Join Date
    Aug 2001
    Location
    Waterloo, Texas
    Posts
    5,600
    Quote Originally Posted by Barney McGrew View Post
    It's necessary for the implementation of the *printf family.
    Okay, so useless to everyone but libc maintainers...
    Code:
    if( numeric_limits< byte >::digits != bits_per_byte )
        error( "program requires bits_per_byte-bit bytes" );
    24bbs.cpp

  10. #10
    Registered User
    Join Date
    Nov 2012
    Posts
    987
    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.

  11. #11
    Stoned Witch Barney McGrew's Avatar
    Join Date
    Oct 2012
    Location
    astaylea
    Posts
    420
    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.

  12. #12
    Registered User
    Join Date
    Nov 2012
    Posts
    987
    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.

  13. #13
    Registered User
    Join Date
    Mar 2010
    Posts
    531
    Quote Originally Posted by Sebastiani View Post
    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!

  14. #14
    Guest Sebastiani's Avatar
    Join Date
    Aug 2001
    Location
    Waterloo, Texas
    Posts
    5,600
    Quote Originally Posted by smokeyangel View Post
    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...
    Last edited by Sebastiani; 02-23-2013 at 04:23 AM.
    Code:
    if( numeric_limits< byte >::digits != bits_per_byte )
        error( "program requires bits_per_byte-bit bytes" );
    24bbs.cpp

  15. #15
    Guest Sebastiani's Avatar
    Join Date
    Aug 2001
    Location
    Waterloo, Texas
    Posts
    5,600

    Exclamation

    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!
    Code:
    if( numeric_limits< byte >::digits != bits_per_byte )
        error( "program requires bits_per_byte-bit bytes" );
    24bbs.cpp

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Rate this code? what could i have done better?
    By SuperMiguel in forum C Programming
    Replies: 23
    Last Post: 07-08-2012, 05:51 PM
  2. Rate my code
    By Baard in forum C++ Programming
    Replies: 2
    Last Post: 01-04-2004, 08:19 AM
  3. rate my directx code
    By revelation437 in forum Game Programming
    Replies: 9
    Last Post: 01-02-2004, 05:21 PM
  4. Rate my source code :)
    By Terrance in forum A Brief History of Cprogramming.com
    Replies: 11
    Last Post: 10-17-2003, 01:09 PM
  5. rate my code
    By kashifk in forum C Programming
    Replies: 1
    Last Post: 06-07-2003, 12:18 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21