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

• Zeta: Which C standard do you want to use? ANSI C? C99? C11?
• Silidrone: I use C99 C standard.

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

• Silidrone: Really good explanation :) I just didn't understandferror(stdin). I don't know what is ferror doing.And the stdin variable you passed both to ferror and to fgets.
• Silidrone: And i wanted to ask,is it wrong (or worse) to put main function firstly and then other functions below it.Of course declaring them will be necessary.
• Silidrone: Too much questions i am asking.But i need one more to sleep well :) I wanted to ask what is strcspn ?? I googled,but i can't find good explanation
• 200_success: stdin is standard input — basically the stream that is associated with keyboard input, but which could also be input from a pipe or a file. It's the same source that getchar() would read from. ferror(stdin) tests whether any error has occurred while trying to read from stdin. strcspn(..., "\r\n") finds the number of characters before the first carriage return or newline is encountered.
• 200_success: It is customary to put main() last, so that you don't have to declare all of the functions that it uses.

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.

• forsvarir: I'd consider null terminating your string in getInput, you've also got a typo when you call it, I'm guessing it should be getInput(buff, ARRSIZE), not getInput(dst...
• Zeta: @forsvarir Good point on getInput(dst.... I would use \NUL, but I'm not sure whether OP wants to use the char[ARRSIZE] as actual strings. Since there is the printArray function, it's possible that \NUL is part of the input. Otherwise one could have used puts(reversed).
• forsvarir: Fair enough, just looks a little odd, because of the /n detection
• Zeta: @forsvarir yeah. Without \n, it would be simply fread(stdin, buff, sizeof char, ARRSIZE - 1). Well, more or less.
• Silidrone: Nice Lesson :) Helpful !