1
\$\begingroup\$

Here is my C code for finding the most occurring word in a sentence:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "ht.h"

#define MAX_WORD_LEN     1024

/*
 * Caller must call free on returned string
 * */
char* find_most_occurring_word(const char* sentence) {
    if (!sentence)
        return NULL;
    if (*sentence == '\0')
        return strdup("");

    ht* words_to_counts;
    char* ptr_word;
    int* ptr_cur_count;
    const char* sep = " ";

    words_to_counts = ht_create();
    if (!words_to_counts) {
        fprintf(stderr, "out of memory");
        exit(EXIT_FAILURE);
    }

    char* dup_sentence = strdup(sentence);

    // tokenize sentence into words
    ptr_word = strtok(dup_sentence, sep);
    while (ptr_word != NULL) {
        ptr_cur_count = ht_get(words_to_counts, ptr_word);

        // set count as 1 if word not exists
        if (ptr_cur_count == NULL) {
            int* int_mem = malloc(sizeof(int));
            *int_mem = 1;
            ht_set(words_to_counts, ptr_word, int_mem);
        }

        // increment count if word exists
        else {
            *ptr_cur_count = *ptr_cur_count + 1;
            ht_set(words_to_counts, ptr_word, ptr_cur_count);
        }

        ptr_word = strtok(NULL, sep);
    }

    // find max occurring word by iterating the hash table
    char max_word[MAX_WORD_LEN];
    int max_val = INT32_MIN;
    int cur_val;
    hti words_to_counts_iterator = ht_iterator(words_to_counts);

    while (ht_next(&words_to_counts_iterator)) {
        printf("key: %s, occurrence: %d\n", words_to_counts_iterator.key, *(int*) words_to_counts_iterator.value);
        cur_val = *((int*) words_to_counts_iterator.value);
        if (cur_val > max_val) {
            max_val = cur_val;
            strcpy(max_word, words_to_counts_iterator.key);
        }
        free(words_to_counts_iterator.value);
    }

    ht_destroy(words_to_counts);
    free(dup_sentence);

    char* result = strdup(max_word);
    return result;
}

int main() {
    const char* sentence = "aa aa aa bb bb bb cc cc cc cc dd dd dd dd dd";
    char* word = find_most_occurring_word(sentence);

    printf("\nword: %s", word);
    free(word);
}

I would like to get a general code review and hear comments and criticism about my code.

The hash table implementation is taken from here.

\$\endgroup\$
2
  • \$\begingroup\$ This does not yet take user input. Is this intentional? \$\endgroup\$
    – Mast
    Dec 6, 2021 at 5:28
  • \$\begingroup\$ @Mast Good catch, that is not intentional, I'll consider getting the input sentence from an external source. \$\endgroup\$
    – ndogac
    Dec 6, 2021 at 7:19

2 Answers 2

2
\$\begingroup\$

Good formatting

Inconsistent Out-of-memory handling

After words_to_counts = ht_create();, code tests and on failure, prints an error message. Yet not so with char* dup_sentence = strdup(sentence);

I recommend in all such places that allocation may fail, simple return NULL. Let the the caller print an error if desired.

Artificial limitation

Yes, MAX_WORD_LEN makes for a quick solution, but the strcpy(max_word, words_to_counts_iterator.key); does not check/enforce that limit. Better code would scale as needed.

Note: as code tokenizes the sentence, keep track of the longest word's size, then the maximum size of all words is know prior to declaring max_word[].

Only spaces?

Rather than const char* sep = " ";, I'd expect any white-space to de-limit.

Size to the reference object, not type

Consider below. The 2nd is right by construction, easier to review and maintain.

int_mem = malloc(sizeof(int));
// vs.
int_mem = malloc(sizeof *int_mem);

Bug: Zero case

If the string is all spaces, code attempts to strdup(strdup(max_word)) with the uninitialized max_word[]: undefined behavior (UB).

Sentinels

Print with a leading and following visible character to more readily detect unexpected white-space (like '\n', '\t').

Print a '\n' at the end, to ensure stdout is printed then. Alternative, use fflush(stdout).

// printf("\nword: %s", word);
printf("word: \"%s\"\n", word);

Types

Code is assuming int and int32_t are the same. Avoid that portability mistake.

// int max_val = INT32_MIN;
int max_val = INT_MIN;

A long string's length may exceed iNT_MAX, yet not SIZE_MAX. Use size_t for string lengths and occurrence count.`

size_t max_val = 0;
...
// int* int_mem = malloc(sizeof(int));
size_t* count_mem = malloc(sizeof *count_mem);

strtok() alternative

Rather than copy the sentence with char* dup_sentence = strdup(sentence);, code could use strspn() and strcspn() to tokenize a const char * maintaining an index and length. Although this may not work with OP's ht...().

Define when needed

Rather than define cur_val outside the loop, define it inside. Better to keep objects definitions and life local as able.

//int cur_val;
//...
//while (...
//    cur_val = *((int*) words_to_counts_iterator.value);

while (...
    int cur_val = *((int*) words_to_counts_iterator.value);

Only 1 test case

\$\endgroup\$
0
\$\begingroup\$

In addition to @chux's comments, I'd point out that you don't need a pass through the hash-table.

You can simply keep track of the current highest count, and the corresponding word.

By doing this you'd also avoid any need for MAX_WORD_LEN...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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