Assignement on reading from stdin in C

Question

I am an experienced Python developer learning C. I solved this assignement:

Write a program that reads a word and some sentences from stdin. The word is separated by space from a sentences. The sentences are separated by one of the following characters: .!?. Words in sentences are separated by any char for which one of library functions isspace and ispunct returns nonzero result.

Your program should output average usage of the word in the sentences (a number of matched words divided by a number of sentences).

Example input:

two one four two seven eight two four two. two, one, nine, two, two, three, two!

Example expected output: 3.5

Explanation: The word is "two". Then we have two sentences: "one four two seven eight two four two." and "two, one, nine, two, two, three, two!". The word occurs 7 times, so result is 7/2 = 3.5.

Here is my solution:

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

void check_malloc_result(void* malloc_result) {
    if (!malloc_result) {
        perror("malloc failed");
        exit(EXIT_FAILURE);
    }
}

int main(void) {
    char stdin_ch;

    size_t buff_size = 8;
    size_t word_len = 0;
    bool reading_word = false;

    char* word = malloc(buff_size);
    check_malloc_result(word);

    // reading the word
    while(read(STDIN_FILENO, &stdin_ch, 1) > 0) {
        if (isspace(stdin_ch)) {
            if (reading_word) {
                break;
            } else {
                continue;
            }
        }
        reading_word = true;
        ++word_len;

        if (word_len > buff_size) {
            buff_size *= 2;
            word = realloc(word, buff_size);
            check_malloc_result(word);
        }

        word[word_len - 1] = stdin_ch;
    }

    // counting matches
    size_t sen_count = 0, matched_count = 0, cur_word_pos = 0;
    bool matching = true;
    reading_word = false;

    while(word_len && read(STDIN_FILENO, &stdin_ch, 1) > 0) {
        if (ispunct(stdin_ch) || isspace(stdin_ch)) {
            if (reading_word) {
                // end of a word
                if (matching) {
                    ++matched_count;
                }
                if (stdin_ch == '.' || stdin_ch == '?' || stdin_ch == '!') {
                    ++sen_count;
                }
                reading_word = false;
                matching = true;
                cur_word_pos = 0;
            }
            continue;
        }

        reading_word = true;
        if (matching) {
            if (cur_word_pos >= word_len || word[cur_word_pos] != stdin_ch) {
                matching = false;
            }
            ++cur_word_pos;
        }
    }
    free(word);

    // printing the result
    double result = 0.0;
    if (sen_count) {
        result = (double)matched_count / (double)sen_count;
    }
    printf("%f", result);

    return EXIT_SUCCESS;
}

Some notes:

• I don't want to use fixed max length for the word so I use dynamic allocation here.
• The reading of sentences is done using two boolean flags. reading_word means are actualy reading a word, and if it's false, we are somewhere in between. It's used for dealing with consecutive separators. matching becomes false when we know that current scanned word doesn't match, so we just scan to the end of the word, not checking anything.

What could be better in this code? Is it "idiomatic" C, or is it obvious that it's written by someone without much language experience?

Answer 1

First, some positive notes. I got no warnings when compiled with gcc using -Wall -Wextra -pedantic. The code seems to work correctly, although the user has to signal EOF from the keyboard, or use file redirection (some warning about this in the question would have been nice.) It is good to see you using size_t for array indices, good that you are checking the results of your functions (i.e., malloc() and realloc()), and good that you are remembering to free() allocated memory.

I like that you do not cast the result of malloc(), though there are those that would disagree on that point.

Concerning realloc(), you are directly assigning the result of realloc() to the pointer to the memory which is being reallocated. This is bad, since realloc() can return a NULL pointer in the event of an error. This would cause a memory leak because you would no longer have a pointer to the previous allocation. You should instead store the result of realloc() in a temporary variable, and test that before reassigning:

char *temp;
...
temp = realloc(word, buff_size);
check_malloc_result(temp);
word = temp;

I don't really understand the need for dynamic allocation here. You only allocate storage for one word; this is not a lot of memory. The method of allocating a reasonable space, and then growing it as needed is sound, but note that if the first word is 17 characters, then you will have to allocate for 32 characters. This is a lot of waste, if you are concerned about space at this scale. You could allocate for one extra character at a time, as needed. I would recommend allocating for a larger initial value (say, 100 chars); if you want to handle possible very large first words, then you can grow this allocation dynamically. Then, if you really want to trim the memory, you can realloc() to the actual length of the word after you have read it.

It would be more idiomatic to use getchar() instead of read() to read from stdin. And, since the specification says that the only separators are spaces and the three punctuation characters, I would consider using '\n' as a way to terminate input to make keyboard entry more friendly:

while((stdin_ch = getchar()) != '\n' && stdin_ch != EOF) {
...
while(word_len && (stdin_ch = getchar()) != '\n' && stdin_ch != EOF) {

Without the test for newlines, this looks much simpler than the read() version of the same code:

while((stdin_ch = getchar()) != EOF) {
...
while(word_len && (stdin_ch = getchar()) != EOF) {

When you calculate result, you don't need to use two casts; the first will do:

result = (double)matched_count / sen_count;

Or, you can do away with the cast altogether by multiplying by 1.0; Note that a floating constant without a suffix is of type double:

result = (1.0 * matched_count) / sen_count;

I personally lean towards the second approach as I prefer to avoid casts whenever possible.

I also added a newline to the format string of your final printf statement, since this is nicer for the user; you might have a reason not to do this, though.

Finally, a note on the indirection operator: opinions vary, but it seems most common to use "right-leaning" asterisks. This is purely a matter of style, but I prefer:

int *var;

to the "left-leaning" version:

int* var;

The only place that I make an exception is in function declarations:

int * func(int *ptr);

There are good reasons for both styles; consistency trumps all in this case.