7
\$\begingroup\$

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, '\0');
}

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;
}
\$\endgroup\$
  • \$\begingroup\$ This approach consumes the trailing delimiter. Its value is lost. Is that truly intended? \$\endgroup\$ – chux Jun 26 '18 at 11:33
  • \$\begingroup\$ @chux That’s pretty standard behaviour for formatted input in C and C++. \$\endgroup\$ – Konrad Rudolph Jun 26 '18 at 11:36
  • \$\begingroup\$ @KonradRudolph In C, it is not. scanf("%s", buf) does not consume trailing white-space after the populating buf. White-space is detected, yet returned to stdin. Similar for scanf("%d", &i) and others. \$\endgroup\$ – chux Jun 26 '18 at 11:46
  • \$\begingroup\$ @chux Fair point. I misremembered. \$\endgroup\$ – Konrad Rudolph Jun 26 '18 at 12:24
14
\$\begingroup\$

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, '\0');
}

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, '\0');
}

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);
}
\$\endgroup\$
  • \$\begingroup\$ thanks for the in deep review. i have to admit i didn't even expected to read much comments to this code because i considered the code to easy and short :-) I was quite wrong... One question i have hwat would be an better error handling? and why exit(1) not exit(-1) \$\endgroup\$ – Sandro4912 Jun 25 '18 at 18:34
  • \$\begingroup\$ One nit to pick. I would change "if (!tmp)" to "if (tmp == NULL)", because tmp is not a logical (true/false) condition, it's a comparison to a special value, the NULL pointer. FALSE and NULL just happen to be implemented as 0 in C, but that's not necessarily universally true. \$\endgroup\$ – jamesqf Jun 25 '18 at 20:33
  • 3
    \$\begingroup\$ @james/hoffmale: It's a standard C (and C++) idiom to test the validity of a pointer by leaving the comparison to 0 implicit. Remember that a literal 0 always means a null pointer (whether or not the machine uses all-bits-zero for its representation). NULL doesn't "just happen" to be implemented as 0 - the Standard insists that it must be 0, even on hardware where the zero address is valid. In those cases, the implementation is required to evaluate a pointer holding the zero address as a "true" value. \$\endgroup\$ – Toby Speight Jun 26 '18 at 7:35
  • 1
  • 1
    \$\begingroup\$ Detail: "0 always means a null pointer" is amiss. 0 is always a constant int with value 0. Comparing that int to a pointer p, converts the int 0 to a null pointer. IAC, if (!tmp) is OK, yet as a minor point, I prefer if (tmp == NULL) as negations tend to be less clear than positive assertions - don't you not think is won't be otherwise ;-)? (Good review) \$\endgroup\$ – chux Jun 26 '18 at 11:41
8
\$\begingroup\$

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 '\0' 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++] = '\0';

    return str;
}

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

    getchar();
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ I was continuing my answer as you wrote this. I didn't think of the better initial capacity, though. \$\endgroup\$ – Toby Speight Jun 25 '18 at 16:57
  • \$\begingroup\$ i think this code reflects more the ximplicity of c. since i do mostly c++ i think i tend to overengineer "easy" tasks \$\endgroup\$ – Sandro4912 Jun 25 '18 at 18:35
  • \$\begingroup\$ @Sandro4912: I don't think this is about "simplicity of C". C programs can very easily become very complex, even more complex than C++ with classes and templates. This is about using the right tool for the job, adhering to general language idioms and not doing more than what is needed. \$\endgroup\$ – hoffmale Jun 25 '18 at 18:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.