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

Red Black Tree Implementation

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

Problem

I would like some feedback on my red black tree implementation. Anything is fine. I've debugged this and it seems to be working fine, however I may have missed something.

Basically, this is a red black tree that stores character strings as keys and the passage that contains those strings as values. Since these keys are able to be repeated, they form a linked list as well.

TNODE *tree_add(TNODE *root, const KEY k, const VALUE v) {
        LNODE *lnode = NULL;
        if (root == NULL) {
                TNODE *node = talloc(k);
                lnode = lalloc(v);
                node->head = lnode;
                node->tail = lnode;
                node->is_red = true;
                return node;
        }
        if (strcmp(k, root->key) left = tree_add(root->left, k, v);
        } else if (strcmp(k, root->key) > 0) {  
                root->right = tree_add(root->right, k, v);
        } else {
                if (strcmp(k, root->key) == 0) {
                        lnode = lalloc(v);
                        root->tail->next = lnode;
                        root->tail = lnode;
                        root->tail->next = NULL;
                }
        }
        if (is_red(root->right) && !is_red(root->left)) {
                root = rotate_left(root);
        }
        if (is_red(root->left) && is_red(root->left->left)) {
                root = rotate_right(root);
        }
        if (is_red(root->left) && is_red(root->right)) {
                flip_colors(root);
        }
        return root;

}


Here are TNODE and LNODE:

```
// LNODE is the data structure for a singly linked list.
typedef struct lnode {
VALUE val; // A pointer to the value stored in the linked list.
struct lnode *next; // Pointer to the next item in the list; it should be NULL if there is no successor.
} LNODE;

typedef struct tnode {
KEY key; // Search key for this binary search tree node.
struct tnode *right; // Right child.
struct tnode *left; // Left

Solution

This isn't quite a full review, but here goes anyway:

-
It looks like you're using 8-space indents. That's quite a lot. I think 4 is more common, but it is a matter of preference and it is consistent, so it's definitely not wrong

-
You have KEY, presumably a typedef of char*, but then you call strcmp on it. If you want it to be general across key types (which is a pretty good idea IMO), you should pass in a comparator function that takes two keys as arguments.

-
I'm going to assume that the other fields of TNODE exist for some reason, but is there a chance you could move them out to rbtree_t and leave TNODE smaller?

-
I don't know the naming conventions for types in C, so don't take this as implicit agreement or disagreement

-
talloc and lalloc probably don't need to be their own functions; malloc(sizeof(TNODE)) is probably sufficient

Context

StackExchange Code Review Q#68461, answer score: 2

Revisions (0)

No revisions yet.