patterncMinor
Red Black Tree Implementation
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.
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
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
-
I'm going to assume that the other fields of
-
I don't know the naming conventions for types in C, so don't take this as implicit agreement or disagreement
-
-
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 sufficientContext
StackExchange Code Review Q#68461, answer score: 2
Revisions (0)
No revisions yet.