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

Binary Search Tree - C++

Submitted by: @import:stackexchange-codereview··
0
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

Solution

You can avoid a lot of ownership problems by using:

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.