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

---

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? :)

Quote:

---

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

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

---

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

Quote:

---

Originally Posted by Richardcavell

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

Quote:

---

Originally Posted by algorism

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

---

:biggrin::biggrin: THANK YOU!