Write a function reverse(s) that reverses the character string s. Use it to write a program that reverses its input a line at a time

Question

Continuing to slowly progress through The C Programming Language by Brian Kernighan and Dennis Ritchie.

The code I came up with below is for Exercise 1-19 - Write a function reverse(s) that reverses the character string s. Use it to write a program that reverses its input a line at a time.

Feedback is much appreciated.

// Exercise 1-19. Write a function reverse(s) that reverses the character string s.
// Use it to write a program that reverses its input a line at a time.

#include <stdio.h>

void reverseLine(char *s);

void reverseLine(char *s) {
    int count;
    count = 0;
    for (int i = 0; s[i] != '\0'; i++) {
        count++;
    }
    for (int j = count - 1; j >= 0; j--) {
        putchar(s[j -1]);
    }
    putchar('\n');
}

int main() {
    int ch, charCount;
    char line[BUFSIZ];
    charCount = 0;

    while ((ch = getchar()) != EOF) {
        if (charCount >= BUFSIZ - 1) {
            fprintf(stderr, "Overly long line ignored.\n");
            while ((ch = getchar()) != EOF && ch != '\n') {
                ;
            }
            line[charCount] = ch;
            charCount = 0;
            continue;
        }
        line[charCount++] = ch;
        if (ch == '\n') {
            line[charCount++] = '\0';
            reverseLine(line);
            charCount = 0;
        }
    }
}

Answer 1

Your implementation is not what the exercise asked for. It asked for a function that reverses a string (implied: in-place), and separately, I/O to read and print lines using that function. What it did not ask for is a function that immediately prints the reverse of the string.

Your declaration of reverseLine is redundant so delete it; only the definition is needed.

In your current intepretation, s should be a const char *s since it isn't mutated, but again, the problem asks for in-place mutation.

Rather than ignoring long lines, just truncate them.

reverseLine should be static.

C allows for int main to omit the return, but that's a bad idea. Write the return.

In-predicate mutation, i.e. while ((ch = getchar()), is common but an anti-pattern. Do not mutate your variables on the inside of predicates.

reverseLine has re-implemented strlen. Don't do that; just call strlen.

Answer 2

Independent of whether the function reverseString as written is doing what it should, it has an off-by-one error. In the loop putcharing the characters of the input string in reverse order,

for (int j = count - 1; j >= 0; j--) {
    putchar(s[j - 1]);
}

consider that for any count > 0, the last iteration will execute with j equal to 0, causing s[-1] to be accessed, which is out-of-bounds. The argument to putchar should be just s[j]. Note also that even with this correction, your loop would fail to terminate if j were an unsigned type (due to underflow). A correct version of this loop that works for both signed and unsigned loop indexes is

for (int j = count; j > 0; ) {
    putchar(s[--j]);
}

Answer 3

The main program can be simplified by using a function you probably haven't read about yet called fgets. This function reads a line of text at a time. The variable line will contain the new line after the function has been called; you would need to remove the newline before you reverse the input.

int main() {
    char line[BUFSIZ];

    while (fgets(line, BUFSIZ, stdin) != NULL) {
        if (line[0]) {
            reverseLine(line);
            printf("%s\n", line);
        }
    }
}

Answer 4

Much focus on how to read a line, yet the coding goal was more on

Write a function reverse(s) that reverses the character string s.

• OP's reverseLine() is not too bad, yet the stated goal does not include printing.

• Strings are rarely, yet could be longer than INT_MAX. To handle all strings use size_t count for determining string length. Alternatively, simply use an end pointer.

    void reverseLine(char *s) {
      // for (int i = 0; s[i] != '\0'; i++) {
      //  count++;
      //}
      char *endptr = s;
      while (*endptr) {
        endptr++;
      }
      // endptr now points to the string's null character.

      // walk the start and end pointers together until they meet.
      while (s < endptr) {
        endptr--; 
        char t = *endptr;
        *endptr = *s;
        *s = t;
        s++;
      }
    }

Often it is useful to consider the 0 length case as in reverseLine(""). OP's code does that OK with int, yet if size_t (an unsigned type), it would have trouble when count == 0 and j = count - 1. Instead test for > 0, and then decrement.

// for (int j = count - 1; j >= 0; j--) {
for (size_t j = count; j > 0; ) {
    j--; // add
    putchar(s[j -1]);

reverseLine() will now reverse a string, as was the coding goal.
Now for the next goal.

Use it to write a program that reverses its input a line at a time.

To reverse the input, OP's code is OK, yet ++ not needed in charCount++. Also, since a revised reverseLine() only reverses the string and not print it, we should print the result.

    if (ch == '\n') {
        //line[charCount++] = '\0';
        line[charCount] = '\0';
        reverseLine(line);
        puts(line); // add
        charCount = 0;

The last line

Under various conditions, the last line before end-of-file might not end with a '\n'. So it makes sense after the while ((ch = getchar()) != EOF) { to test for a rump line.

while ((ch = getchar()) != EOF) {
  ...
}

if (charCount > 0) {
  line[charCount] = '\0';
  reverseLine(line);
  puts(line); // Add if reverseLine() does not print.
}
  

Answer 5

For your edification, here's a relatively efficient program which fits the exercise and imposes no specific limit in line length nor in number of lines.

A few notes:

1. The program assumes Linux-style line endings (aka LF, not CRLF).
2. The program's efficiency could be improved by calling fgets directly on string.data.
   • But to avoid allocation in the common case, it would make String slightly more complicated.
3. The program uses // instead of /**/ for comments, this is not strictly compliant, but much easier.
4. The program has been compiled cleanly with -Wall -Wextra -Werror, but has not been tested.

Without further ado [and on godbolt](https://godbolt.org/z/3ozh8z8j4:

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

//  In-place reverses the string `s` of length `length`.
static void reverse(char* s, size_t length) {
    for (size_t i = 0; i < length / 2; ++i) {
        char temp = s[i];
        s[i] = s[length - 1 - i];
        s[length - 1 - i] = temp;
    }
}

struct String {
    //  NUL-terminated, if non-NULL.
    char* data;
    //  Length exclusive of NUL-terminator.
    size_t length;
    //  Allocated size of the `data`.
    size_t capacity;
};

//  Increase the capacity of `this->data` to be able to fit at least `extra` characters.
static void str_grow(struct String* this, size_t extra) {
    size_t new_capacity = this->capacity > 0 ? this->capacity * 2 : BUFSIZ * 2;

    while (extra > new_capacity - this->length) {
        new_capacity *= 2;
    }

    this->data = this->capacity == 0 ? malloc(new_capacity) : realloc(this->data, new_capacity);

    if (this->data == NULL) {
        fprintf(stderr, "Out of memory");
        exit(1);
    }

    this->capacity = new_capacity;
}

//  Appends the first `length` characters of `s` to `this`.
static void str_append(struct String* this, char const* s, size_t length) {
    if (length > this->capacity - this->length) {
        str_grow(this, length);
    }

    memcpy(this->data + this->length, s, length);
    this->length += length;
    this->data[this->length] = '\0';
}

int main() {
    char buffer[BUFSIZ];
    struct String string = {};

    while (fgets(buffer, sizeof(buffer), stdin) != NULL) {
        size_t read = strlen(buffer);
        assert(read > 0 && "fgets returns either when the input is exhausted, the buffer is full, or when \n is encountered");

        //  Accumulate into heap-allocated buffer if the line is incomplete, and keep reading.
        if (read != BUFSIZ - 1 && buffer[read - 1] != '\n') {
            str_append(&string, buffer, read);
            continue;
        }

        //  Trim the newline character as it shouldn't be reversed.
        //  Portability note: assumes end of lines are LF only, on Windows they may be CRLF instead.
        if (buffer[read - 1] == '\n') {
            buffer[read - 1] = '\0';
            read -= 1;
        }

        //  Speed up common case of short lines by avoiding the copy to `string.data`.
        //  Also avoids memory allocations altogether if all lines are short.
        if (string.length == 0) {
            reverse(buffer, read);

            printf("%s\n", buffer);

            continue;
        }

        str_append(&string, buffer, read);

        reverse(string.data, string.length);

        printf("%s\n", string.data);

        //  Reset string length after use, next read will be a new line.
        string.length = 0;
    }

    free(string.data);
}

Answer 6

Write a function reverse(s) that reverses the character string s. Use it to write a program that reverses its input a line at a time

OP's code has a line length of BUFSIZ.
Alternative, use recursion, without a line length limit - at least until stack depth is reached.

static int reverse_helper(void) {
  int ch = getchar();
  if (ch != EOF && ch != '\n') {
    int ch2 = reverse_helper();
    putchar(ch);
    ch = ch2;
  }
  return ch;
}

int reverse(void) {
  int ch = reverse_helper();
  if (ch == '\n') {
    putchar(ch);
  }
  fflush(stdout);
  return ch;
}

int main(void) {
  while (reverse() != EOF)
    ;
}