Anatomy of a string-reversal program with less coupling

Question

I'm currently trying to improve my C understanding. Would you mind telling me if there are good/bad things that I did here? (I implemented a function to reverse a string in place).

For example things I feel uncomfortable with:

• I forwarded pointers to a buffer so that I could loosen the coupling of different parts of the program, is it a bad thing to forward many times a pointer?
• I would like to set the functions I extracted to a header, somehow but I don't know if this is stupid or not

Here is what I came up with so far with my main.c file (the actual implementation of reverse_str is not what I want to focus on so I did not include it):

#include "reverse_str.h"
const long unsigned MAX_BUFFER_SIZE = 100;
void remove_new_line(char *buffer, const unsigned length)
{
 long unsigned buffer_last_index = length - 1;
 if (buffer[buffer_last_index] == '\n')
 buffer[buffer_last_index] = '0円';
}
char * get_buffer()
{
 char *buffer = malloc(MAX_BUFFER_SIZE * sizeof (*buffer));
 return buffer;
}
unsigned read_input(char *buffer, const long unsigned size)
{
 fgets(buffer, size, stdin);
 return strlen(buffer);
}
char * get_input()
{
 char *buffer = get_buffer();
 const unsigned length = read_input(buffer, MAX_BUFFER_SIZE);
 remove_new_line(buffer, length);
 return buffer;
}
char * ask_for_input()
{
 printf("Enter a string, whatever\n");
 char *buffer = get_input();
 return buffer;
}
void manipulate(char * buffer)
{
 reverse_str(buffer);
}
void show_result(char *buffer)
{
 printf("After: %s\n", buffer);
}
// I know it is a bit redundant, but it keeps symetry with ask_for_input
void clear(char *buffer)
{
 free(buffer);
}
int main()
{
 char *buffer = ask_for_input();
 manipulate(buffer);
 show_result(buffer);
 clear(buffer);
 return 0;
}

Answer 1

• When you're passing a buffer around, you should always pass the size of the buffer (or the desired size of the buffer) as well. Your get_buffer() function that creates a buffer should return both the buffer and the buffer's size (either in a struct or as separate parameters), so that the caller of the function knows how much data is safe to put in it. (Yes, you've got that global constant, but what if you move to a paradigm where the allocator can allocate different-sized buffers?)
• When you're using a buffer to hold a variable-length string,, you should pass the string length around as well, so you don't have to repeatedly call strlen(). Again you'll need to create a new struct or use separate parameters. (削除) fgets() returns the size of the string. (削除ここまで) Since fgets() doesn't return the length of the string, you do have to do a single strlen() (and you should, to make sure that you got the whole string ending with the newline).
• I don't understand why you have a function that creates a buffer, but no function that destroy a buffer. Symmetric structure is good. The current setup locks you in to using malloc() and free(). If you wanted to switch to another memory allocation method, there's no simple way to do it.

• axelduch: OK so for the first point: what kind of structure should I return to hold both the data and the meta data, a struct? For the second point: it appears that fgets() returns a char * (i.e. the buffer) Lastly: I totally agree with this point, I should have seen it, I will do this.
• Snowbody: Oops! You're right. I was thinking of something else. You do have to do a single strlen().
• Snowbody: Please don't edit the code in your question. codereview.se's policy is if you change your code, post a new question.
• axelduch: I'm sorry I was unaware of that. I reverted my update