HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Template system for FastCGI app in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fastcgitemplateappsystemfor

Problem

I'm doing a small FastCGI application in C and I've created this template system to keep the design away from the code.

There's a binary tree containing all possible first characters, each node points to another binary tree containing all characters that can appear after it. Some nodes have the replacement, when they are reached it means that a match was found.

The template system will read the characters from the file and parse the tree at the same time, always favoring the longest match.

I would like to know how to improve efficiency and reduce complexity.

word_tree.h

#ifndef WORD_TREE_H
#define WORD_TREE_H

typedef enum {
    WT_SUCCESS, WT_ERROR, WT_DUPLICATE, WT_DONE //WT_DONE is for internal use only
} WT_Return;

typedef struct Word_Tree{
    struct Word_Tree *sequence;
    struct Word_Tree *smaller;
    struct Word_Tree *greater;
    const char *replacement;
    int c;
} Word_Tree;

void word_tree_init(Word_Tree **root);
void word_tree_delete(Word_Tree *root);

WT_Return word_tree_insert(Word_Tree **root, const char *word, const char *replacement);

const Word_Tree *word_tree_find(const Word_Tree *wt, int c);

#endif


word_tree.c

```
#include "word_tree.h"
#include

void word_tree_init(Word_Tree **root)
{
*root = NULL;
}

static Word_Tree *word_tree_new(void)
{
Word_Tree *wt = malloc(sizeof(Word_Tree));
if(wt == NULL)
return NULL;

wt->sequence = NULL;
wt->smaller = NULL;
wt->greater = NULL;
wt->replacement = NULL;
wt->c = '\0';

return wt;
}

void word_tree_delete(Word_Tree *root)
{
if(root->sequence)
word_tree_delete(root->sequence);

if(root->smaller)
word_tree_delete(root->smaller);

if(root->greater)
word_tree_delete(root->greater);

free(root);
}

WT_Return word_tree_insert(Word_Tree **root, const char word, const char replacement)
{
int comparison;

//Signal that the replacement should be inserted
if(*word == '\0')
return WT_DONE;

Solution

A couple notes:

-
This looks fishy to me:

//Try to find the character
if(comparison > 0){
    switch(word_tree_insert(&(*root)->greater, word, replacement)){
        case WT_DUPLICATE:
            return WT_DUPLICATE;

        case WT_ERROR:
            return WT_ERROR;

        case WT_SUCCESS:
            return WT_SUCCESS;

        case WT_DONE:
            if((*root)->replacement != NULL)
                return WT_DUPLICATE;

            (*root)->replacement = replacement;
            return WT_SUCCESS;
    }
}


It seems like a very complex system when you mostly just return the
case values (except for one specific value). Here is what I would do
to clean it up (untested):

//Try to find the character
if(comparison > 0)
{
    WT_Return val = word_tree_insert(&(*root)->greater, word, replacement);
    if(val != WT_DONE) return val;
    else
    {
        if((*root)->replacement) return WT_DUPLICATE;
        (*root)->replacement = replacement;
        return WT_SUCCESS;
    }
}


-
You have a comment here:

//If we are inserting, it's impossible to be a duplicate


I think that you should be using assert() in this one situation to verify that, since it is used to check for situations that "can't happen" or are "impossible".

Code Snippets

//Try to find the character
if(comparison > 0){
    switch(word_tree_insert(&(*root)->greater, word, replacement)){
        case WT_DUPLICATE:
            return WT_DUPLICATE;

        case WT_ERROR:
            return WT_ERROR;

        case WT_SUCCESS:
            return WT_SUCCESS;

        case WT_DONE:
            if((*root)->replacement != NULL)
                return WT_DUPLICATE;

            (*root)->replacement = replacement;
            return WT_SUCCESS;
    }
}
//Try to find the character
if(comparison > 0)
{
    WT_Return val = word_tree_insert(&(*root)->greater, word, replacement);
    if(val != WT_DONE) return val;
    else
    {
        if((*root)->replacement) return WT_DUPLICATE;
        (*root)->replacement = replacement;
        return WT_SUCCESS;
    }
}
//If we are inserting, it's impossible to be a duplicate

Context

StackExchange Code Review Q#54678, answer score: 6

Revisions (0)

No revisions yet.