Reverse input program in C99

Question

I am writing simple program to reverse its input and output the reversed version. I came up with this code. So how can I improve it? Something tells me that int lengthSoFar is bad in both printArray and reverse functions.

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

#define ARRSIZE 1000

//function declarations...
char *reverse(char *a, int l);

void printArray(char *a, int l);

int main(void) {
    int c = 0;
    int i = 0;
    char buff[ARRSIZE];
    while ((c = getchar()) != EOF) {
        if (i > ARRSIZE) printArray(reverse(buff, i), i);
        if (c != '\n') {
            buff[i]=c;
            i++;
        }else{
            printArray(reverse(buff, i), i);
            memset(buff, 0, sizeof buff);
            i=0;
        }
    }
    return 0;
}

char *reverse(char *array, int lengthSoFar) {
    int i = 0;
    int j = lengthSoFar - 1;
    static char arrayToReturn[ARRSIZE];
    while (i < lengthSoFar && j >= 0) {
        arrayToReturn[i] = array[j];
        i++;
        j--;
    }
    return arrayToReturn;
}

void printArray(char *array, int lengthSoFar) {
    for (int i = 0; i < lengthSoFar; i++) {
        putchar(array[i]);
    }
}

Answer 1

Since this program reads its input one line at a time, I would naturally expect it to print its output line by line as well. Instead, it prints no newlines at all, which is surprising to me.

---

When dealing with buffers, it is usually beneficial to test whether the program works as intended with a tiny buffer. (Alternatively, you could feed it excessively long input, but reducing the buffer size is easier.) If I reduce ARRSIZE to 10, then give it hello there as input, I get repeated output followed by a crash:

ereht ollehereht ollehAbort trap: 6

So, you have some bugs with the if (i > ARRSIZE) printArray(reverse(buff, i), i); logic. I'll leave it to you to figure out what went wrong.

---

You memset() it to 0 after printing, but not initially. So, it looks like you are under the impression that

char buff[ARRSIZE];

gives you an array whose contents are all initially 0. However, zeroed memory is not guaranteed by the C language specification. In practice, most modern operating systems will zero all of your memory before your program runs, as a security precaution so that you can't peek at potentially sensitive junk that a previously executed program left behind in memory. However, to be strictly correct, you would only be guaranteed a blank slate if you wrote one of the following:

char buff[ARRSIZE] = { 0 };
char buff[ARRSIZE] = "";
static char buff[ARRSIZE];

I don't recommend a static buffer for this application.

---

Your reverse() function returns a static character buffer. I don't recommend that design for two reasons:

• The buffer size is predetermined, such that it's not a general-purpose string-reversing function that works with any input.
• static buffers are error-prone designs. If you call char *a = reverse(foo), then char *b = reverse(bar), it could surprise you that a ends up containing the reversed contents of bar. Such side-effects are undesirable.

Based on the observation that the lengths of the input and output are the same, I would design reverse() to accept an in-out parameter. That is, instead of returning a reversed copy of its input, it just operates on the buffer directly. Though in-out parameters may feel like an unnatural hack, they are often the best way in C to avoid memory-management headaches.

---

This program is supposed to process one line at a time. Therefore, the code would be clearer if it worked a line at a time rather than a character at a time. For example, you can read a whole line using fgets(), and you can print a whole line using puts().

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

#define ARRSIZE 1000

/* Writes a NUL terminator at the first carriage return or newline
   (if any).  Returns the length of the (likely truncated) string. */
size_t chomp(char *line) {
    size_t len = strcspn(line, "\r\n");
    line[len] = '\0';
    return len;
}

/* Reverses a buffer of len bytes in place.  Returns a pointer to
   the same buffer. */
char *reverse(char *buffer, size_t len) {
    for (char *a = buffer, *b = buffer + len - 1; a < b; a++, b--) {
        char swap = *a;
        *a = *b;
        *b = swap;
    }
    return buffer;
}

int main(void) {
    char line[ARRSIZE];
    while (fgets(line, sizeof line, stdin)) {
        size_t len = chomp(line);
        puts(reverse(line, len));
    }
    return ferror(stdin);
}

Answer 2

Buffer overflow

There are two problems with your current attempt to prevent buffer overflow:

    if (i > ARRSIZE) printArray(reverse(buff, i), i);

The first is that your code is off by one, and if i is larger than ARRSIZE you have already overflowed your buffer by one character. So you should change > to >=. The second is that once you detect the overflow, you don't set i back to zero. So you will continue to write past the end of your buffer.

Answer 3

Take a destination for the reversed string

Instead of a local pointer, make reverse a void function:

void reverse(char * dst, const char * src, size_t length) {
    for(size_t i = 0; i < length; ++i) {
        dst[i] = src[length - i - 1];
    }
}

The names dst (destination) and src (source) and their order stem from standard library functions like strncpy.

size_t is an unsigned integer type, and large enough to index an array that spans your whole memory. We're allowed to define size_t i in the for loop, therefore our variable doesn't live any longer than necessary.

But you already did that for printArray, so I guess you knew that :)

void printArray(const char * array, size_t length) {
    for (size_t i = 0; i < length; i++) {
        putchar(array[i]);
    }
}

It's good practice to use const <type> * if you don't want to change the data by accident.

Use a function to ask for the string

To keep your main sleek and your code more reusable, provide a function to get the string from the user instead:

int getInput(char * dst, size_t max_len) {
    int length = 0;
    int c;

    while((c = getchar()) != EOF && length < max_len) {
        if(c == '\n')
            return length;

        dst[length++] = c;
    }
    return c == EOF ? EOF : length;
}

This will return EOF if getchar() returned EOF, and the number of read characters otherwise.

Putting all back together

I omit the functions I've already covered above, but if we use them, we end up with the following program:

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

#define ARRSIZE 1000

int getInput(char * dst, size_t max_len);
void reverse(char * dst, const char * src, size_t length);
void printArray(const char * array, size_t length);

int main(void) {
    int length = 0;
    char buff[ARRSIZE];
    char reversed[ARRSIZE];

    while ((length = getInput(buff, ARRSIZE)) != EOF) {
        reverse(buff, reversed, length);
        printArray(reversed, length);
    }
    return 0;
}

Which, in my opinion, is rather readable. Note that your memset wasn't necessary. After all, we're going to overwrite the values in buff with the next input.