patterncppMinorCanonical
Binary Search Tree - C++
Viewed 0 times
binarytreesearch
Problem
I'm implementing several data structures in an attempt to learn C++. Below is a binary search tree that I've implemented to learn about pointers, dangling pointers, and memory leaks. I was hoping someone could critique my code, point out any problems that they may find, or any inconsistencies. Please, be as harsh you feel necessary.
Note: As far as I know this implementation works well. Although, I feel as if the remove function could be simplified some how.
BinarySearchTree.h
```
//
// BinarySearchTree.h
// Data Structures
#ifndef __Data_Structures__BinarySearchTree__
#define __Data_Structures__BinarySearchTree__
#include
#include
#pragma mark - Enumerations
typedef enum : int
{
TraversalTypeInOrder,
TraversalTypePreOrder,
TraversalTypePostOrder
} TraversalType;
#pragma mark -
#pragma mark - Class Definition
template
class BinarySearchTree
{
#pragma mark - Structures
template
struct Node
{
Key key;
Node * left = nullptr;
Node * right = nullptr;
Node * parent = nullptr;
};
#pragma mark -
#pragma mark - Private Member Variables
Node * root = nullptr;
#pragma mark -
#pragma mark - Private Helper Functions
T minimum(Node * node)
{
if (node->left == nullptr)
return node->key;
return minimum(node->left);
}
T maximum(Node * node)
{
if (node->right == nullptr)
return node->key;
return maximum(node->right);
}
#pragma mark -
#pragma mark - Private Action Functions
void insert(const T &key, Node * node)
{
if (key key)
{
if (node->left)
{
insert(key, node->left);
}
else {
Node * left = new Node();
left->key = key;
left->parent = node;
node->left = left;
}
}
else {
if (node->right)
{
insert
Note: As far as I know this implementation works well. Although, I feel as if the remove function could be simplified some how.
BinarySearchTree.h
```
//
// BinarySearchTree.h
// Data Structures
#ifndef __Data_Structures__BinarySearchTree__
#define __Data_Structures__BinarySearchTree__
#include
#include
#pragma mark - Enumerations
typedef enum : int
{
TraversalTypeInOrder,
TraversalTypePreOrder,
TraversalTypePostOrder
} TraversalType;
#pragma mark -
#pragma mark - Class Definition
template
class BinarySearchTree
{
#pragma mark - Structures
template
struct Node
{
Key key;
Node * left = nullptr;
Node * right = nullptr;
Node * parent = nullptr;
};
#pragma mark -
#pragma mark - Private Member Variables
Node * root = nullptr;
#pragma mark -
#pragma mark - Private Helper Functions
T minimum(Node * node)
{
if (node->left == nullptr)
return node->key;
return minimum(node->left);
}
T maximum(Node * node)
{
if (node->right == nullptr)
return node->key;
return maximum(node->right);
}
#pragma mark -
#pragma mark - Private Action Functions
void insert(const T &key, Node * node)
{
if (key key)
{
if (node->left)
{
insert(key, node->left);
}
else {
Node * left = new Node();
left->key = key;
left->parent = node;
node->left = left;
}
}
else {
if (node->right)
{
insert
Solution
You can avoid a lot of ownership problems by using:
and also simplify a lot of logic into loops by using:
(but still
You should only use
I have never had cause to use anything but in-order traversals, and those are implemented by iterators.
All of your
In your many of your functions, you are wasting stack with recursion unless the compiler performs TCE.
You are missing copy/move constructor/assignment operators.
This is not a self-balancing binary tree, so it will degenerate to a linked list on certain common inputs.
std::unique_ptr> root;and also simplify a lot of logic into loops by using:
enum { LEFT, RIGHT };
std::unique_ptr> children[2];(but still
Node *parent; being because it's backwards, not owning)You should only use
operator instead of #include srand seeds a poor random number generator, and time is a very predictable seed. But you don't actually call rand in this code.I have never had cause to use anything but in-order traversals, and those are implemented by iterators.
All of your
#pragma mark is really distracting. Most tools are sensible enough to use comments for documentation grouping, but most of the time that is only needed on a very coarse basis at namespace scope.In your many of your functions, you are wasting stack with recursion unless the compiler performs TCE.
You are missing copy/move constructor/assignment operators.
This is not a self-balancing binary tree, so it will degenerate to a linked list on certain common inputs.
Code Snippets
std::unique_ptr<Node<T>> root;enum { LEFT, RIGHT };
std::unique_ptr<Node<T>> children[2];Context
StackExchange Code Review Q#97075, answer score: 9
Revisions (0)
No revisions yet.