View Poll Results: How good is my code?

Voters
2. You may not vote on this poll
  • One star

    0 0%
  • Two stars

    1 50.00%
  • Three stars

    1 50.00%

Thread: Please rate my code

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

    Talking Please rate my code

    Hello, everyone. I'm a self-taught C programmer and I'm trying to learn to write code as professionally as possible. This program works, but could you give me a rating as to how well-written it is? TIA.

    Code:
    /* Replace one string with another in copying stdin to stdout. */
    
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    void
    clear(char *sz_buffer, size_t bufsize, unsigned int *ui_bufpos)
    {
      *ui_bufpos = 0;
      (void) memset(sz_buffer, 0, bufsize);
    }
    
    void
    output_error(void)
    {
      (void) fprintf(stderr, "Error: Couldn't put character to stdout.\n");
      exit(EXIT_FAILURE);
    }
    
    void
    print_string(const char *s)
    {
      if (printf("%s", s) < 0)
        output_error();
    }
    
    void
    print_char(const char c)
    {
      if (EOF == putchar(c))
        output_error();
    }
    
    int main(int argc, char *argv[])
    {
      int c = EOF;
      size_t buffer_size = 0;
      char *sz_buffer = NULL;
      unsigned int ui_bufpos = 0;
    
      if (argc != 3)
      {
        (void) fprintf(stderr, "Usage : %s before after\n",
                (argv[0]) ? argv[0] : "replace");
        exit(EXIT_FAILURE);
      }
    
      buffer_size = strlen(argv[1]) + 1; /* for the terminating \0 */
      sz_buffer = malloc(buffer_size);
    
      if (NULL == sz_buffer)
      {
        (void) fprintf(stderr, "Error: Out of memory.\n");
        exit(EXIT_FAILURE);
      }
    
      clear(sz_buffer, buffer_size, &ui_bufpos);
    
      while ((c = getchar()) != EOF)
      {
        if (ui_bufpos < buffer_size - 1)
        {
          sz_buffer[ui_bufpos++] = c;
        }
        else
        {
          print_char(sz_buffer[0]);
          (void) memmove(sz_buffer, sz_buffer + 1, buffer_size - 2);
          sz_buffer[buffer_size - 2] = c;
        }
    
        if ( (ui_bufpos == buffer_size - 1) &&
             (0 == strcmp(sz_buffer, argv[1])) )
        {
          print_string(argv[2]);
          clear(sz_buffer, buffer_size, &ui_bufpos);
        }
      }
    
      print_string(sz_buffer);
      free(sz_buffer);
    
      return EXIT_SUCCESS;
    }

  2. #2
    Its hard... But im here swgh's Avatar
    Join Date
    Apr 2005
    Location
    England
    Posts
    1,688
    Looks pretty good. Clean and concise code with good indentation and current standard function calls. One very slight thing, the way you declare your functions seem a little strange:

    Code:
    void
    exampleFunction()
    {
    }
    I've never seen that sort of declaration before. Is it a new practice?
    Double Helix STL

  3. #3
    Registered User
    Join Date
    May 2010
    Posts
    4,633
    I've never seen that sort of declaration before. Is it a new practice?
    No it's been around for a while. The gnu group seems to be one of the practitioners of this style. See this link. It's not one of my favorite styles, but if you use this style you should use it everywhere, including main(). I'm also not a big fan of always casting the unused return values to (void). Also if you get a failure writing to stdout, trying to output to stderr may also fail.

    What is the significance of the sz_, ui_ prefixes? If they are to indicate something about the type of variable, I say that this is a not so good practice, to me it just obfuscates the code for no real gain.

    Look at this snippet:
    Code:
      size_t buffer_size = 0;
      ...
      unsigned int ui_bufpos = 0;
    ...
        if (ui_bufpos < buffer_size - 1)
    You have an unsigned int and a size_t and later in your program you compare the values of these two variables. A couple of things to note, a size_t is an implementation defined unsigned type, it could be an unsigned long long, unsigned long, unsigned int. So ui_bufpos really should be a size_t not an unsigned int.

    Also if it is ever possible for buffer_size to be zero subtracting - 1 will be problematic, remember there are no negative values so you will get the largest possible value for the type. You really should be insuring that these values will never underflow.

    Lastly I'm not a fan of the following:
    Code:
    0 == strcmp(sz_buffer, argv[1]
    To me it "reads" backwards. With today's compilers you really don't gain anything with this syntax.

    Jim

  4. #4
    Registered User
    Join Date
    Jun 2015
    Posts
    1,640
    My main comment is with your algorithm. The buffer is unnecessary.

    If you did need to use a buffer then you should encapsulate the buffer variables in a struct. And making it a circular buffer would obviate the constant memmoving. (But, as I said, the buffer is not needed.)

    I would also like to add my vote AGAINST the so-called Hungarian notation. It's pointless and ugly.

    I also dislike writing comparisons backwards. That was a fad for a while, but it doesn't gain you anything if you maximize your compiler warnings.
    Code:
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    
    int main(int argc, char **argv) {
    
        if (argc != 3) {
            fprintf(stderr, "Usage: replace search_text replacement_text\n");
            exit(EXIT_FAILURE);
        }
    
        const char *search_text = argv[1];
        const char *replacement_text = argv[2];
        const char *p = search_text;
        const char * const end = p + strlen(search_text);
        int c;
    
        while ((c = getchar()) != EOF) {
            if (c == *p) {
                if (++p == end) {
                    fputs(replacement_text, stdout);
                    p = search_text;
                }
            }
            else {
                for (const char *q = search_text; q < p; q++)
                    putchar(*q);
                p = search_text;
                putchar(c);
            }
        }
    
        for (const char *q = search_text; q < p; q++)
            putchar(*q);
    
        return 0;
    }
    Last edited by algorism; 01-04-2017 at 05:10 PM.

  5. #5
    Registered User
    Join Date
    Feb 2011
    Posts
    144

    Thanks all

    I agree that the buffer is unnecessary. Thanks for picking me up on that, and to all who replied. But your code is broken, algorism. Try doing "$replace run walk" with input of "overrun".

  6. #6
    Registered User
    Join Date
    Jun 2015
    Posts
    1,640
    Quote Originally Posted by Richardcavell View Post
    your code is broken, algorism. Try doing "$replace run walk" with input of "overrun".
    I literally wrote and "tested" it in five minutes, so I don't doubt it's broken. I leave it as an exercise for you to fix it. (It's easy.)

  7. #7
    Old Fashioned
    Join Date
    Nov 2016
    Posts
    137
    Quote Originally Posted by algorism View Post
    I would also like to add my vote AGAINST the so-called Hungarian notation. It's pointless and ugly.
    THANK YOU!

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Please rate at my code
    By Richardcavell in forum C Programming
    Replies: 16
    Last Post: 02-23-2013, 02:34 PM
  2. 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
  3. Rate my code
    By Baard in forum C++ Programming
    Replies: 2
    Last Post: 01-04-2004, 09:19 AM
  4. rate my directx code
    By revelation437 in forum Game Programming
    Replies: 9
    Last Post: 01-02-2004, 06:21 PM
  5. rate my code
    By kashifk in forum C Programming
    Replies: 1
    Last Post: 06-07-2003, 12:18 PM

Tags for this Thread