I did the following exercise:

Write a C program that does the equivalent of C++ string s; cin>>s;; that is, define an input operation that reads an arbitrarily long sequence of whitespace-terminated characters into a zero terminated array of chars.

I wonder if it's good code. What could be improved?

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>

struct String 
 char* signs;
 size_t size;
 size_t capacity;
;

void String_allocate_space(char **c, size_t *capacity)

 if (*capacity == 0) // allocate the first time
 *capacity = 1;
 *c = malloc(sizeof(**c) * ((*capacity)));
 
 else 
 *capacity *= 2; // double the new capacity
 *c = realloc(*c, sizeof(**c) * (*capacity));
 
 if (*c == NULL)
 exit(-1);


void add_character(struct String* string, int ch)

 if (string->size == string->capacity) // if current letter sz = capacity
 String_allocate_space(&string->signs, &string->capacity);
 
 string->signs[string->size++] = ch; // append the sign in the array


void String_read(struct String* string)

 int ch = ' ';
 while (ch = getc(stdin)) 

 if (!isalpha(ch))
 break;

 add_character(string, ch);
 
 add_character(string, '');


void String_print(struct String* string)

 printf("%s", string->signs);


void String_free(struct String* string)

 int i = 0;
 for (i = 0; i < string->capacity; ++i) 
 free(string[i].signs);
 string[i].signs = NULL;
 


int main()

 struct String string = 0 ;

 String_read(&string);
 String_print(&string);
 String_free(&string);

 getchar();
 return 0;

Don't do this:

*c = realloc(*c, sizeof(**c) * (*capacity));

Once you have error handling that's more sophisticated than exit(1), this will become a liability. You need a temporary:

char *tmp = realloc(*c, new_capacity);
if (!tmp) 
 /* error handling - c is still valid */
 /* ... */

*c = tmp;
*capacity = new_capacity;

If you always initialize the data pointer to start as a null pointer, you don't need to use malloc() instead of realloc(). String_allocate_space would be easier to write if it accepts a pointer to a struct String; that makes it closer to the object-oriented version:

void String_init(struct String* string)

 string->data = NULL;
 string->size = string->capacity = 0;


void String_allocate_space(struct String* string)

 size_t new_capacity = string->capacity ? 2 * string->capacity : 16;
 char *tmp = realloc(string->data, new_capacity);
 if (!tmp) 
 /* error handling - c is still valid */
 exit(1); /* TODO: improve error reporting */
 
 string->data = tmp;
 string->capacity = new_capacity;

I've also incorporated a change above to start with a larger initial size (16) instead of 1. That lets us skip the first 4 reallocations for free.

The read() method has a flaw that will become apparent when you try to read another value into a string - unlike std::string, reading with >> will append to it, instead of replacing it. We need to reset size at the beginning:

void String_read(struct String* string)

 string->size = 0;
 int ch;
 while (ch = getc(stdin)) 

 if (!isalpha(ch))
 break;

 add_character(string, ch);
 
 add_character(string, '');

Also, the logic is slightly wrong - we want to finish when we see a space, rather than any non-alpha (which could be digits or punctuation characters). (Well done for remembering that getch() returns int rather than char - that's one common error avoided).

void String_read(struct String* string)

 string->size = 0;
 int ch;
 while ((ch = getc(stdin)) != EOF && !isspace(ch)) 
 add_character(string, (char)ch);
 
 add_character(string, '');

When we free the string, we don't need a loop. Instead, we have a single free(). It's a good idea to reset the size and capacity so that the string object is consistent - it can be used again and/or freed again without harm:

void String_free(struct String* string)

 free(string->data);
 string->data = NULL;
 string->size = string->capacity = 0;

This is an important concept in object-oriented programming - objects have invariants that they guarantee are true at the start and end of every (public) operation. In this case, the invariants are that

• 
  data points to valid storage of at least capacity if capacity > 0 and is a null pointer otherwise.


• 
  size is not greater than capacity.

We can improve the printing so that it outputs any embedded NUL characters, just like C++ strings do:

void String_print(struct String* string)

 if (fwrite(string->data, 1, string->size, stdout) != string->size) 
 exit(1); /* TODO: improve error reporting */
 

Note that this will now print the trailing NUL we added. We no longer need that to mark the end of string, so we can remove that line.

Modified code

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>

struct String 
 char* data;
 size_t size;
 size_t capacity;
;

void String_init(struct String* string)

 string->data = NULL;
 string->size = string->capacity = 0;


void String_allocate_space(struct String* string)

 size_t new_capacity = string->capacity ? 2 * string->capacity : 1;
 char *tmp = realloc(string->data, new_capacity);
 if (!tmp) 
 /* error handling - c is still valid */
 exit(1); /* TODO: improve error reporting */
 
 string->data = tmp;
 string->capacity = new_capacity;


void add_character(struct String* string, char ch)

 if (string->size == string->capacity) // if current letter exceeds capacity
 String_allocate_space(string);
 
 string->data[string->size++] = ch; // append it


void String_read(struct String* string)

 string->size = 0;
 int ch;
 while ((ch = getc(stdin)) != EOF && !isspace(ch)) 
 add_character(string, (char)ch);
 


void String_print(struct String *restrict string, FILE *restrict stream)

 if (fwrite(string->data, 1, string->size, stream) != string->size) 
 exit(1); /* TODO: improve error reporting */
 


void String_free(struct String* string)

 free(string->data);
 string->data = NULL;
 string->size = string->capacity = 0;


int main()

 struct String string;

 String_init(&string);
 String_read(&string);
 String_print(&string, stdout);
 String_free(&string);

Since @TobySpeight has already posted a wonderful answer, I'm not going to repeat what he has already posted.

Just some small additional notes:

• String_allocate_space allocates 1 chars worth of memory if the capacity is 0. This is barely enough to hold the terminating '' character, but nothing more. Maybe increase the default minimal allocation size a bit to be meaningful?




• Also, regarding naming: If I see a function called String_free, I'd expect a function String_alloc that allocates and creates the String object in a well-defined state. (And consequently, String_free should then deallocate that String correctly.)

Also, this might just me, but from the task description I'd expect the solution to be a char *read_input(void) function. While the String "class" is nice, it seems like a bit of over-engineering for the task at hand.

For comparison, look at this solution:

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>

#define INITIAL_INPUT_CAPACITY 4

char *read_input(void) 
 size_t capacity = INITIAL_INPUT_CAPACITY;
 size_t size = 0;
 char *str = malloc(capacity * sizeof(char));
 int input;

 while((input = getc(stdin)) != EOF) 
 if(isspace(input)) break;

 str[size++] = (char)input;

 if(size == capacity) 
 capacity *= 2;
 char *temp = realloc(str, capacity * sizeof(char));

 if(temp == NULL) 
 exit(-1);
 

 str = temp;
 
 

 str[size++] = '';

 return str;


int main(void) 
 char *input = read_input();
 puts(input);
 free(input);

 getchar();
 return 0;