Skip to main content
3 of 6
deleted 4 characters in body
Madagascar
  • 10.1k
  • 1
  • 16
  • 52

Do not cast the return of malloc() and family:

Starting with C89, malloc() and family returns a generic void * (as compared to the char * it originally used to) that is implicitly converted to any other pointer type (the cast is redundant, no need to clutter the codebase).

#if 0
Node *node = (Node *)calloc(1,sizeof(Node));
#else
// Take the size from the variable rather than its type to reduce the chance of a mismatch.
Node *const node = calloc(1, sizeof *node);
#endif

Side-note: Is there any specific reason you used calloc() for node but malloc() for node->children?

Check the return value of library functions:

If a function be advertised to return an error code in the event of difficulties, thou shalt check for that code, yea, even though the checks triple the size of thy code and produce aches in thy typing fingers, for if thou thinkest ''it cannot happen to me'', the gods shall surely punish thee for thy arrogance. - The Ten Commandments for C Programmers

According to the C17 standard:

The malloc function returns either a null pointer or a pointer to the allocated space.

Failing to check for it risks invoking undefined behavior by a subsequent null pointer dereference.

#if 0
    Node * node = (Node *)calloc(1,sizeof(Node));

    node->children = (Node **)malloc(DEFAULT_LENGTH_OF_CHILDREN_ARRAY*sizeof(Node*));
#else
    Node *const node = calloc(1, sizeof *node);

    if (!node) {
        return NULL;
    }

    node->children = malloc(DEFAULT_LENGTH_OF_CHILDREN_ARRAY * sizeof node->children[0]);
    
    if (!node->children) {
        free(node);
        return NULL;
    }
    ...
}
#endif

Use braces around if/while/for statement et cetera:

I suggest always using:

if (current_node->number_of_children > 0) {
    current_node->is_end_of_word = false;
else {
    delete_branch(parent_of_branch_to_delete, index_of_branch_to_delete);
}

to

if (current_node->number_of_children > 0)
    current_node->is_end_of_word = false;
else
    delete_branch(parent_of_branch_to_delete, index_of_branch_to_delete);

The problem with the second version is that if you go back and add a second statement to the if or else clause without adding the curly braces, your code will break. See: Apple's SSL/TLS bug.

Use size_t for sizes, cardinalities, and ordinal numbers:


    int length_of_children_array; /*< Length of allocated children array 
                                    (NB: this is not the number of children, just the memory allocated for
                                    their pointers.) 
                                */
    int number_of_children; /*< Number of children of the node. */

The length_of_children_array and number_of_children can not be negative. Consider using size_t or some other unsigned type.

Minor:

/*
    Free the children array of the given node from memory and then free the node itself.
*/
void delete_node(Node *node) {
    free(node->children);
    free(node);
}

I don't think this comment adds any value to the code. It is perfectly clear what delete_node() does. Consider removing it.

Madagascar
  • 10.1k
  • 1
  • 16
  • 52