Binary search tree with common operations
I trying to see if I implemented these operations correctly and if they could be optimized in any way. The implementations are just the typical implementations found online but I adjusted them to work with pointers. I did not find pseudocode for transplant, it was only described and I am unsure if it does what it is supposed to do. The tree works correctly for my small test cases, but I have not tried it with bigger data sets and operations. I purposely made the tree non-owning but if it were owning, left and right children could be unique_ptr
's and parent be a Node*
. I may implement that later.
class Node {
public:
Node(int key) : key(key) {}
Node* parent = nullptr;
Node* left = nullptr;
Node* right = nullptr;
int key;
};
class BinarySearchTree {
public:
BinarySearchTree() = default;
/// create tree from existing nodes
BinarySearchTree(Node* nodes, unsigned n);
/// insert key into tree
void insert(Node* node);
/// remove node with key
void remove(Node* node);
/// find smallest node in subtree
Node* minimum(Node* node) const;
/// find largest node in subtree
Node* maximum(Node* node) const;
/// find successor of node
Node* successor(Node* node) const;
/// find predecessor of node
Node* predecessor(Node* node) const;
/// replace subtree at a with subtree at b
void transplant(Node* a, Node* b);
/// find node with key
Node* find(int key) const;
/// find kth smallest element
Node* kth(unsigned k) const;
/// write data to stream
friend std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree);
private:
/// helper method to initialize tree
void build_bst(BinarySearchTree& tree, Node* nodes, unsigned n);
/// helper method for find
Node* find(int key, Node* subtree) const;
/// pointer to the root
Node* root = nullptr;
};
void BinarySearchTree::build_bst(BinarySearchTree& tree, Node* nodes, unsigned n) {
if (n == 0) return;
tree.insert(nodes[n/2]);
build_bst(tree, nodes, n/2);
build_bst(tree, nodes + n/2 + 1, n - 1 - n/2);
}
BinarySearchTree::BinarySearchTree(Node* nodes, unsigned n) {
build_bst(*this, nodes, n);
}
void BinarySearchTree::insert(Node* node) {
Node* parent = nullptr;
Node* child = root;
while (child) {
parent = child;
if (node->key < child->key)
child = child->left;
else
child = child->right;
}
node->parent = parent;
if (!parent) {
root = node;
} else if (node->key < parent->key) {
parent->left = node;
} else {
parent->right = node;
}
}
void BinarySearchTree::remove(Node* node) {
if (!node->left && !node->right) {
// special case where only node is root
if (node->parent) {
// remove leaf node
if (node->parent->key < node->key) {
node->parent->right = nullptr;
} else {
node->parent->left = nullptr;
}
} else {
root = nullptr;
}
node->parent = nullptr;
} else if (!node->left && node->right) {
// no left child
transplant(node, node->right);
} else if (node->left && !node->right) {
// no right child
transplant(node, node->left);
} else {
// check if min = successor(node)
Node* min = successor(node);
if (min->parent != node) {
transplant(min, min->right);
min->right = node->right;
min->right->parent = min;
}
transplant(node, min);
min->left = node->left;
min->left->parent = min;
}
}
Node* BinarySearchTree::minimum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->left;
}
return parent;
}
Node* BinarySearchTree::maximum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->right;
}
return parent;
}
Node* BinarySearchTree::successor(Node* node) const {
if (node->right) {
// find minimum of right subtree
return minimum(node->right);
} else {
// find parent of furthest node through right branches
Node* iter = node;
while (iter->parent) {
if (iter->parent->key < iter->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no successor
return iter->parent;
}
}
Node* BinarySearchTree::predecessor(Node* node) const {
if (node->left) {
return maximum(node->left);
} else {
// find parent of furthest node through left branches
Node* iter = node;
while (iter->parent) {
if (iter->key < iter->parent->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no predecessor exists
return iter->parent;
}
}
void BinarySearchTree::transplant(Node* a, Node* b) {
if (b->parent->key < b->key) {
b->parent->right = nullptr;
} else {
b->parent->left = nullptr;
}
b->parent = a->parent;
// special case when a is root
if (a->parent) {
if (a->parent->key < a->key) {
a->parent->right = b;
} else {
a->parent->left = b;
}
} else {
root = b;
}
}
Node* BinarySearchTree::find(int key) const {
return find(key, root);
}
Node* BinarySearchTree::find(int key, Node* subtree) const {
if (!subtree) return nullptr;
if (subtree->key < key)
return find(key, subtree->right);
else if (subtree->key > key)
return find(key, subtree->left);
else
return subtree;
}
Node* BinarySearchTree::kth(unsigned k) const {
Node* kth = minimum(root);
for (unsigned i = 0; i < k-1; ++i) {
if (kth)
kth = successor(kth);
else
return kth;
}
// returns nullptr if kth element does not exist
return kth;
}
/// in-order traversal/print
std::ostream& operator<<(std::ostream& out, const Node& node) {
if (node.left)
out << *node.left;
out << node.key << ' ';
if (node.right)
out << *node.right;
return out;
}
/// print the entire tree
std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree) {
if (!tree.root) return out;
return out << *tree.root;
}
c++ tree binary-search
add a comment |
I trying to see if I implemented these operations correctly and if they could be optimized in any way. The implementations are just the typical implementations found online but I adjusted them to work with pointers. I did not find pseudocode for transplant, it was only described and I am unsure if it does what it is supposed to do. The tree works correctly for my small test cases, but I have not tried it with bigger data sets and operations. I purposely made the tree non-owning but if it were owning, left and right children could be unique_ptr
's and parent be a Node*
. I may implement that later.
class Node {
public:
Node(int key) : key(key) {}
Node* parent = nullptr;
Node* left = nullptr;
Node* right = nullptr;
int key;
};
class BinarySearchTree {
public:
BinarySearchTree() = default;
/// create tree from existing nodes
BinarySearchTree(Node* nodes, unsigned n);
/// insert key into tree
void insert(Node* node);
/// remove node with key
void remove(Node* node);
/// find smallest node in subtree
Node* minimum(Node* node) const;
/// find largest node in subtree
Node* maximum(Node* node) const;
/// find successor of node
Node* successor(Node* node) const;
/// find predecessor of node
Node* predecessor(Node* node) const;
/// replace subtree at a with subtree at b
void transplant(Node* a, Node* b);
/// find node with key
Node* find(int key) const;
/// find kth smallest element
Node* kth(unsigned k) const;
/// write data to stream
friend std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree);
private:
/// helper method to initialize tree
void build_bst(BinarySearchTree& tree, Node* nodes, unsigned n);
/// helper method for find
Node* find(int key, Node* subtree) const;
/// pointer to the root
Node* root = nullptr;
};
void BinarySearchTree::build_bst(BinarySearchTree& tree, Node* nodes, unsigned n) {
if (n == 0) return;
tree.insert(nodes[n/2]);
build_bst(tree, nodes, n/2);
build_bst(tree, nodes + n/2 + 1, n - 1 - n/2);
}
BinarySearchTree::BinarySearchTree(Node* nodes, unsigned n) {
build_bst(*this, nodes, n);
}
void BinarySearchTree::insert(Node* node) {
Node* parent = nullptr;
Node* child = root;
while (child) {
parent = child;
if (node->key < child->key)
child = child->left;
else
child = child->right;
}
node->parent = parent;
if (!parent) {
root = node;
} else if (node->key < parent->key) {
parent->left = node;
} else {
parent->right = node;
}
}
void BinarySearchTree::remove(Node* node) {
if (!node->left && !node->right) {
// special case where only node is root
if (node->parent) {
// remove leaf node
if (node->parent->key < node->key) {
node->parent->right = nullptr;
} else {
node->parent->left = nullptr;
}
} else {
root = nullptr;
}
node->parent = nullptr;
} else if (!node->left && node->right) {
// no left child
transplant(node, node->right);
} else if (node->left && !node->right) {
// no right child
transplant(node, node->left);
} else {
// check if min = successor(node)
Node* min = successor(node);
if (min->parent != node) {
transplant(min, min->right);
min->right = node->right;
min->right->parent = min;
}
transplant(node, min);
min->left = node->left;
min->left->parent = min;
}
}
Node* BinarySearchTree::minimum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->left;
}
return parent;
}
Node* BinarySearchTree::maximum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->right;
}
return parent;
}
Node* BinarySearchTree::successor(Node* node) const {
if (node->right) {
// find minimum of right subtree
return minimum(node->right);
} else {
// find parent of furthest node through right branches
Node* iter = node;
while (iter->parent) {
if (iter->parent->key < iter->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no successor
return iter->parent;
}
}
Node* BinarySearchTree::predecessor(Node* node) const {
if (node->left) {
return maximum(node->left);
} else {
// find parent of furthest node through left branches
Node* iter = node;
while (iter->parent) {
if (iter->key < iter->parent->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no predecessor exists
return iter->parent;
}
}
void BinarySearchTree::transplant(Node* a, Node* b) {
if (b->parent->key < b->key) {
b->parent->right = nullptr;
} else {
b->parent->left = nullptr;
}
b->parent = a->parent;
// special case when a is root
if (a->parent) {
if (a->parent->key < a->key) {
a->parent->right = b;
} else {
a->parent->left = b;
}
} else {
root = b;
}
}
Node* BinarySearchTree::find(int key) const {
return find(key, root);
}
Node* BinarySearchTree::find(int key, Node* subtree) const {
if (!subtree) return nullptr;
if (subtree->key < key)
return find(key, subtree->right);
else if (subtree->key > key)
return find(key, subtree->left);
else
return subtree;
}
Node* BinarySearchTree::kth(unsigned k) const {
Node* kth = minimum(root);
for (unsigned i = 0; i < k-1; ++i) {
if (kth)
kth = successor(kth);
else
return kth;
}
// returns nullptr if kth element does not exist
return kth;
}
/// in-order traversal/print
std::ostream& operator<<(std::ostream& out, const Node& node) {
if (node.left)
out << *node.left;
out << node.key << ' ';
if (node.right)
out << *node.right;
return out;
}
/// print the entire tree
std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree) {
if (!tree.root) return out;
return out << *tree.root;
}
c++ tree binary-search
add a comment |
I trying to see if I implemented these operations correctly and if they could be optimized in any way. The implementations are just the typical implementations found online but I adjusted them to work with pointers. I did not find pseudocode for transplant, it was only described and I am unsure if it does what it is supposed to do. The tree works correctly for my small test cases, but I have not tried it with bigger data sets and operations. I purposely made the tree non-owning but if it were owning, left and right children could be unique_ptr
's and parent be a Node*
. I may implement that later.
class Node {
public:
Node(int key) : key(key) {}
Node* parent = nullptr;
Node* left = nullptr;
Node* right = nullptr;
int key;
};
class BinarySearchTree {
public:
BinarySearchTree() = default;
/// create tree from existing nodes
BinarySearchTree(Node* nodes, unsigned n);
/// insert key into tree
void insert(Node* node);
/// remove node with key
void remove(Node* node);
/// find smallest node in subtree
Node* minimum(Node* node) const;
/// find largest node in subtree
Node* maximum(Node* node) const;
/// find successor of node
Node* successor(Node* node) const;
/// find predecessor of node
Node* predecessor(Node* node) const;
/// replace subtree at a with subtree at b
void transplant(Node* a, Node* b);
/// find node with key
Node* find(int key) const;
/// find kth smallest element
Node* kth(unsigned k) const;
/// write data to stream
friend std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree);
private:
/// helper method to initialize tree
void build_bst(BinarySearchTree& tree, Node* nodes, unsigned n);
/// helper method for find
Node* find(int key, Node* subtree) const;
/// pointer to the root
Node* root = nullptr;
};
void BinarySearchTree::build_bst(BinarySearchTree& tree, Node* nodes, unsigned n) {
if (n == 0) return;
tree.insert(nodes[n/2]);
build_bst(tree, nodes, n/2);
build_bst(tree, nodes + n/2 + 1, n - 1 - n/2);
}
BinarySearchTree::BinarySearchTree(Node* nodes, unsigned n) {
build_bst(*this, nodes, n);
}
void BinarySearchTree::insert(Node* node) {
Node* parent = nullptr;
Node* child = root;
while (child) {
parent = child;
if (node->key < child->key)
child = child->left;
else
child = child->right;
}
node->parent = parent;
if (!parent) {
root = node;
} else if (node->key < parent->key) {
parent->left = node;
} else {
parent->right = node;
}
}
void BinarySearchTree::remove(Node* node) {
if (!node->left && !node->right) {
// special case where only node is root
if (node->parent) {
// remove leaf node
if (node->parent->key < node->key) {
node->parent->right = nullptr;
} else {
node->parent->left = nullptr;
}
} else {
root = nullptr;
}
node->parent = nullptr;
} else if (!node->left && node->right) {
// no left child
transplant(node, node->right);
} else if (node->left && !node->right) {
// no right child
transplant(node, node->left);
} else {
// check if min = successor(node)
Node* min = successor(node);
if (min->parent != node) {
transplant(min, min->right);
min->right = node->right;
min->right->parent = min;
}
transplant(node, min);
min->left = node->left;
min->left->parent = min;
}
}
Node* BinarySearchTree::minimum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->left;
}
return parent;
}
Node* BinarySearchTree::maximum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->right;
}
return parent;
}
Node* BinarySearchTree::successor(Node* node) const {
if (node->right) {
// find minimum of right subtree
return minimum(node->right);
} else {
// find parent of furthest node through right branches
Node* iter = node;
while (iter->parent) {
if (iter->parent->key < iter->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no successor
return iter->parent;
}
}
Node* BinarySearchTree::predecessor(Node* node) const {
if (node->left) {
return maximum(node->left);
} else {
// find parent of furthest node through left branches
Node* iter = node;
while (iter->parent) {
if (iter->key < iter->parent->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no predecessor exists
return iter->parent;
}
}
void BinarySearchTree::transplant(Node* a, Node* b) {
if (b->parent->key < b->key) {
b->parent->right = nullptr;
} else {
b->parent->left = nullptr;
}
b->parent = a->parent;
// special case when a is root
if (a->parent) {
if (a->parent->key < a->key) {
a->parent->right = b;
} else {
a->parent->left = b;
}
} else {
root = b;
}
}
Node* BinarySearchTree::find(int key) const {
return find(key, root);
}
Node* BinarySearchTree::find(int key, Node* subtree) const {
if (!subtree) return nullptr;
if (subtree->key < key)
return find(key, subtree->right);
else if (subtree->key > key)
return find(key, subtree->left);
else
return subtree;
}
Node* BinarySearchTree::kth(unsigned k) const {
Node* kth = minimum(root);
for (unsigned i = 0; i < k-1; ++i) {
if (kth)
kth = successor(kth);
else
return kth;
}
// returns nullptr if kth element does not exist
return kth;
}
/// in-order traversal/print
std::ostream& operator<<(std::ostream& out, const Node& node) {
if (node.left)
out << *node.left;
out << node.key << ' ';
if (node.right)
out << *node.right;
return out;
}
/// print the entire tree
std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree) {
if (!tree.root) return out;
return out << *tree.root;
}
c++ tree binary-search
I trying to see if I implemented these operations correctly and if they could be optimized in any way. The implementations are just the typical implementations found online but I adjusted them to work with pointers. I did not find pseudocode for transplant, it was only described and I am unsure if it does what it is supposed to do. The tree works correctly for my small test cases, but I have not tried it with bigger data sets and operations. I purposely made the tree non-owning but if it were owning, left and right children could be unique_ptr
's and parent be a Node*
. I may implement that later.
class Node {
public:
Node(int key) : key(key) {}
Node* parent = nullptr;
Node* left = nullptr;
Node* right = nullptr;
int key;
};
class BinarySearchTree {
public:
BinarySearchTree() = default;
/// create tree from existing nodes
BinarySearchTree(Node* nodes, unsigned n);
/// insert key into tree
void insert(Node* node);
/// remove node with key
void remove(Node* node);
/// find smallest node in subtree
Node* minimum(Node* node) const;
/// find largest node in subtree
Node* maximum(Node* node) const;
/// find successor of node
Node* successor(Node* node) const;
/// find predecessor of node
Node* predecessor(Node* node) const;
/// replace subtree at a with subtree at b
void transplant(Node* a, Node* b);
/// find node with key
Node* find(int key) const;
/// find kth smallest element
Node* kth(unsigned k) const;
/// write data to stream
friend std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree);
private:
/// helper method to initialize tree
void build_bst(BinarySearchTree& tree, Node* nodes, unsigned n);
/// helper method for find
Node* find(int key, Node* subtree) const;
/// pointer to the root
Node* root = nullptr;
};
void BinarySearchTree::build_bst(BinarySearchTree& tree, Node* nodes, unsigned n) {
if (n == 0) return;
tree.insert(nodes[n/2]);
build_bst(tree, nodes, n/2);
build_bst(tree, nodes + n/2 + 1, n - 1 - n/2);
}
BinarySearchTree::BinarySearchTree(Node* nodes, unsigned n) {
build_bst(*this, nodes, n);
}
void BinarySearchTree::insert(Node* node) {
Node* parent = nullptr;
Node* child = root;
while (child) {
parent = child;
if (node->key < child->key)
child = child->left;
else
child = child->right;
}
node->parent = parent;
if (!parent) {
root = node;
} else if (node->key < parent->key) {
parent->left = node;
} else {
parent->right = node;
}
}
void BinarySearchTree::remove(Node* node) {
if (!node->left && !node->right) {
// special case where only node is root
if (node->parent) {
// remove leaf node
if (node->parent->key < node->key) {
node->parent->right = nullptr;
} else {
node->parent->left = nullptr;
}
} else {
root = nullptr;
}
node->parent = nullptr;
} else if (!node->left && node->right) {
// no left child
transplant(node, node->right);
} else if (node->left && !node->right) {
// no right child
transplant(node, node->left);
} else {
// check if min = successor(node)
Node* min = successor(node);
if (min->parent != node) {
transplant(min, min->right);
min->right = node->right;
min->right->parent = min;
}
transplant(node, min);
min->left = node->left;
min->left->parent = min;
}
}
Node* BinarySearchTree::minimum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->left;
}
return parent;
}
Node* BinarySearchTree::maximum(Node* node) const {
Node* parent = nullptr;
Node* child = node;
while (child) {
parent = child;
child = child->right;
}
return parent;
}
Node* BinarySearchTree::successor(Node* node) const {
if (node->right) {
// find minimum of right subtree
return minimum(node->right);
} else {
// find parent of furthest node through right branches
Node* iter = node;
while (iter->parent) {
if (iter->parent->key < iter->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no successor
return iter->parent;
}
}
Node* BinarySearchTree::predecessor(Node* node) const {
if (node->left) {
return maximum(node->left);
} else {
// find parent of furthest node through left branches
Node* iter = node;
while (iter->parent) {
if (iter->key < iter->parent->key) {
iter = iter->parent;
} else {
break;
}
}
// will return nullptr if no predecessor exists
return iter->parent;
}
}
void BinarySearchTree::transplant(Node* a, Node* b) {
if (b->parent->key < b->key) {
b->parent->right = nullptr;
} else {
b->parent->left = nullptr;
}
b->parent = a->parent;
// special case when a is root
if (a->parent) {
if (a->parent->key < a->key) {
a->parent->right = b;
} else {
a->parent->left = b;
}
} else {
root = b;
}
}
Node* BinarySearchTree::find(int key) const {
return find(key, root);
}
Node* BinarySearchTree::find(int key, Node* subtree) const {
if (!subtree) return nullptr;
if (subtree->key < key)
return find(key, subtree->right);
else if (subtree->key > key)
return find(key, subtree->left);
else
return subtree;
}
Node* BinarySearchTree::kth(unsigned k) const {
Node* kth = minimum(root);
for (unsigned i = 0; i < k-1; ++i) {
if (kth)
kth = successor(kth);
else
return kth;
}
// returns nullptr if kth element does not exist
return kth;
}
/// in-order traversal/print
std::ostream& operator<<(std::ostream& out, const Node& node) {
if (node.left)
out << *node.left;
out << node.key << ' ';
if (node.right)
out << *node.right;
return out;
}
/// print the entire tree
std::ostream& operator<<(std::ostream& out, const BinarySearchTree& tree) {
if (!tree.root) return out;
return out << *tree.root;
}
c++ tree binary-search
c++ tree binary-search
edited Sep 26 at 13:39
asked Sep 26 at 2:25
Brady Dean
30417
30417
add a comment |
add a comment |
1 Answer
1
active
oldest
votes
Good stuff
- Consistent indentation
/// documenting comments
(I applaud to that)- What it does first, how it does it last
(although I personally prefer inline bodies, this is nice example of the separation being done right)
Design
What could be the reason to have the nodes exposed and not owned?
The tree should, in my opinion, accept and expose only values and be a template:
template<class T>
class BinarySearchTree {
struct Node {
std::unique_ptr<Node> left = nullptr;
std::unique_ptr<Node> right = nullptr;
T key;
Node(T key) : key(std::move(key)) {}
};
public:
BinarySearchTree(std::initializer_list<T> init);
You should notice that:
- It uses
std::unique_ptr
because there is simply no excuse now! If you, for whatever reason, need something likeNode*
then it should rather beiterator
or node handle (C++17). - It has constructor accepting std::initializer_list to enable construction like
BinarySearchTree<int>{1,2,3}
- No
parent
inside node. I will elaborate this a bit more:
I can understand why you did this, because I have done it during my first exam and it was not received well by the teacher. You can use stacks tracking the path from root to current node (maybe make it part of the iterator if needed) to implement every exposed operation. I know it is easier to have the parent
in the node, but it should not be there.
Balancing the tree
You have provided constructor (BinarySearchTree(Node* nodes, unsigned n)
using build_bst(BinarySearchTree& tree, Node* nodes, unsigned n)
) that will produce well-balanced tree if the input array is sorted (should probably check and sort if needed), but did not make the tree read-only, which means, that you should provide some means of rebalancing the tree. Try reading about Red-Black Tree (and maybe AVL Tree as alternative).
Personal story: The purpose of the exam was to show understanding of pointers to handle joining two trees or heaps. The teacher could not imagine that somebody would even think about optimal solution... ehm, I did, and received some unwanted attention for the rest of my studies for doing so while helping myself using that parent
pointer instead of three stacks - there was simply not enough time to pull this out... and I was not supposed to even think about such solution, you know :D I hope you now understand that I am not stressing the removal of parent
too much :)
Thanks for the response. This was for my data structures class and we were expected to have the parent pointer. I have modified this code further and I could try implementing without the parent pointer
– Brady Dean
Sep 28 at 19:43
If the only purpose of the review was to check the code before you submit it to the teacher, then, well, it looks good for the level I am assuming you are at. You can testinsert
andremove
a bit more, to be sure, but I did not spot any obvious flaw in the implementation.
– firda
Sep 28 at 20:12
In your personal story, the parent pointer implementation was the optimal solution or wasn’t? I can see how it saves memory. I’m also implementing an avl right now
– Brady Dean
Sep 28 at 21:04
@BradyDean: Theparent
pointer did not change the speed, stack would produce same Big-O (some logarithm in it probably). I have used the pointer to even be able to write it ON PAPER IN TIME. The teacher expected something less-optimal (maybe quadratic complexity) but much easier to write (just tear the nodes from one tree/heap one at a time and put them in the bigger one). I was the only one to even try something smarter/faster. But adding that pointer to the node to make it easier to implement just made the teacher, ehm, mad at me.
– firda
Sep 28 at 21:11
Our professor haven’t even mentioned implementations without the parent pointer and hasn’t talked about using a stack when iterating through. I like the idea tho
– Brady Dean
Sep 28 at 21:14
|
show 1 more comment
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f204378%2fbinary-search-tree-with-common-operations%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
Good stuff
- Consistent indentation
/// documenting comments
(I applaud to that)- What it does first, how it does it last
(although I personally prefer inline bodies, this is nice example of the separation being done right)
Design
What could be the reason to have the nodes exposed and not owned?
The tree should, in my opinion, accept and expose only values and be a template:
template<class T>
class BinarySearchTree {
struct Node {
std::unique_ptr<Node> left = nullptr;
std::unique_ptr<Node> right = nullptr;
T key;
Node(T key) : key(std::move(key)) {}
};
public:
BinarySearchTree(std::initializer_list<T> init);
You should notice that:
- It uses
std::unique_ptr
because there is simply no excuse now! If you, for whatever reason, need something likeNode*
then it should rather beiterator
or node handle (C++17). - It has constructor accepting std::initializer_list to enable construction like
BinarySearchTree<int>{1,2,3}
- No
parent
inside node. I will elaborate this a bit more:
I can understand why you did this, because I have done it during my first exam and it was not received well by the teacher. You can use stacks tracking the path from root to current node (maybe make it part of the iterator if needed) to implement every exposed operation. I know it is easier to have the parent
in the node, but it should not be there.
Balancing the tree
You have provided constructor (BinarySearchTree(Node* nodes, unsigned n)
using build_bst(BinarySearchTree& tree, Node* nodes, unsigned n)
) that will produce well-balanced tree if the input array is sorted (should probably check and sort if needed), but did not make the tree read-only, which means, that you should provide some means of rebalancing the tree. Try reading about Red-Black Tree (and maybe AVL Tree as alternative).
Personal story: The purpose of the exam was to show understanding of pointers to handle joining two trees or heaps. The teacher could not imagine that somebody would even think about optimal solution... ehm, I did, and received some unwanted attention for the rest of my studies for doing so while helping myself using that parent
pointer instead of three stacks - there was simply not enough time to pull this out... and I was not supposed to even think about such solution, you know :D I hope you now understand that I am not stressing the removal of parent
too much :)
Thanks for the response. This was for my data structures class and we were expected to have the parent pointer. I have modified this code further and I could try implementing without the parent pointer
– Brady Dean
Sep 28 at 19:43
If the only purpose of the review was to check the code before you submit it to the teacher, then, well, it looks good for the level I am assuming you are at. You can testinsert
andremove
a bit more, to be sure, but I did not spot any obvious flaw in the implementation.
– firda
Sep 28 at 20:12
In your personal story, the parent pointer implementation was the optimal solution or wasn’t? I can see how it saves memory. I’m also implementing an avl right now
– Brady Dean
Sep 28 at 21:04
@BradyDean: Theparent
pointer did not change the speed, stack would produce same Big-O (some logarithm in it probably). I have used the pointer to even be able to write it ON PAPER IN TIME. The teacher expected something less-optimal (maybe quadratic complexity) but much easier to write (just tear the nodes from one tree/heap one at a time and put them in the bigger one). I was the only one to even try something smarter/faster. But adding that pointer to the node to make it easier to implement just made the teacher, ehm, mad at me.
– firda
Sep 28 at 21:11
Our professor haven’t even mentioned implementations without the parent pointer and hasn’t talked about using a stack when iterating through. I like the idea tho
– Brady Dean
Sep 28 at 21:14
|
show 1 more comment
Good stuff
- Consistent indentation
/// documenting comments
(I applaud to that)- What it does first, how it does it last
(although I personally prefer inline bodies, this is nice example of the separation being done right)
Design
What could be the reason to have the nodes exposed and not owned?
The tree should, in my opinion, accept and expose only values and be a template:
template<class T>
class BinarySearchTree {
struct Node {
std::unique_ptr<Node> left = nullptr;
std::unique_ptr<Node> right = nullptr;
T key;
Node(T key) : key(std::move(key)) {}
};
public:
BinarySearchTree(std::initializer_list<T> init);
You should notice that:
- It uses
std::unique_ptr
because there is simply no excuse now! If you, for whatever reason, need something likeNode*
then it should rather beiterator
or node handle (C++17). - It has constructor accepting std::initializer_list to enable construction like
BinarySearchTree<int>{1,2,3}
- No
parent
inside node. I will elaborate this a bit more:
I can understand why you did this, because I have done it during my first exam and it was not received well by the teacher. You can use stacks tracking the path from root to current node (maybe make it part of the iterator if needed) to implement every exposed operation. I know it is easier to have the parent
in the node, but it should not be there.
Balancing the tree
You have provided constructor (BinarySearchTree(Node* nodes, unsigned n)
using build_bst(BinarySearchTree& tree, Node* nodes, unsigned n)
) that will produce well-balanced tree if the input array is sorted (should probably check and sort if needed), but did not make the tree read-only, which means, that you should provide some means of rebalancing the tree. Try reading about Red-Black Tree (and maybe AVL Tree as alternative).
Personal story: The purpose of the exam was to show understanding of pointers to handle joining two trees or heaps. The teacher could not imagine that somebody would even think about optimal solution... ehm, I did, and received some unwanted attention for the rest of my studies for doing so while helping myself using that parent
pointer instead of three stacks - there was simply not enough time to pull this out... and I was not supposed to even think about such solution, you know :D I hope you now understand that I am not stressing the removal of parent
too much :)
Thanks for the response. This was for my data structures class and we were expected to have the parent pointer. I have modified this code further and I could try implementing without the parent pointer
– Brady Dean
Sep 28 at 19:43
If the only purpose of the review was to check the code before you submit it to the teacher, then, well, it looks good for the level I am assuming you are at. You can testinsert
andremove
a bit more, to be sure, but I did not spot any obvious flaw in the implementation.
– firda
Sep 28 at 20:12
In your personal story, the parent pointer implementation was the optimal solution or wasn’t? I can see how it saves memory. I’m also implementing an avl right now
– Brady Dean
Sep 28 at 21:04
@BradyDean: Theparent
pointer did not change the speed, stack would produce same Big-O (some logarithm in it probably). I have used the pointer to even be able to write it ON PAPER IN TIME. The teacher expected something less-optimal (maybe quadratic complexity) but much easier to write (just tear the nodes from one tree/heap one at a time and put them in the bigger one). I was the only one to even try something smarter/faster. But adding that pointer to the node to make it easier to implement just made the teacher, ehm, mad at me.
– firda
Sep 28 at 21:11
Our professor haven’t even mentioned implementations without the parent pointer and hasn’t talked about using a stack when iterating through. I like the idea tho
– Brady Dean
Sep 28 at 21:14
|
show 1 more comment
Good stuff
- Consistent indentation
/// documenting comments
(I applaud to that)- What it does first, how it does it last
(although I personally prefer inline bodies, this is nice example of the separation being done right)
Design
What could be the reason to have the nodes exposed and not owned?
The tree should, in my opinion, accept and expose only values and be a template:
template<class T>
class BinarySearchTree {
struct Node {
std::unique_ptr<Node> left = nullptr;
std::unique_ptr<Node> right = nullptr;
T key;
Node(T key) : key(std::move(key)) {}
};
public:
BinarySearchTree(std::initializer_list<T> init);
You should notice that:
- It uses
std::unique_ptr
because there is simply no excuse now! If you, for whatever reason, need something likeNode*
then it should rather beiterator
or node handle (C++17). - It has constructor accepting std::initializer_list to enable construction like
BinarySearchTree<int>{1,2,3}
- No
parent
inside node. I will elaborate this a bit more:
I can understand why you did this, because I have done it during my first exam and it was not received well by the teacher. You can use stacks tracking the path from root to current node (maybe make it part of the iterator if needed) to implement every exposed operation. I know it is easier to have the parent
in the node, but it should not be there.
Balancing the tree
You have provided constructor (BinarySearchTree(Node* nodes, unsigned n)
using build_bst(BinarySearchTree& tree, Node* nodes, unsigned n)
) that will produce well-balanced tree if the input array is sorted (should probably check and sort if needed), but did not make the tree read-only, which means, that you should provide some means of rebalancing the tree. Try reading about Red-Black Tree (and maybe AVL Tree as alternative).
Personal story: The purpose of the exam was to show understanding of pointers to handle joining two trees or heaps. The teacher could not imagine that somebody would even think about optimal solution... ehm, I did, and received some unwanted attention for the rest of my studies for doing so while helping myself using that parent
pointer instead of three stacks - there was simply not enough time to pull this out... and I was not supposed to even think about such solution, you know :D I hope you now understand that I am not stressing the removal of parent
too much :)
Good stuff
- Consistent indentation
/// documenting comments
(I applaud to that)- What it does first, how it does it last
(although I personally prefer inline bodies, this is nice example of the separation being done right)
Design
What could be the reason to have the nodes exposed and not owned?
The tree should, in my opinion, accept and expose only values and be a template:
template<class T>
class BinarySearchTree {
struct Node {
std::unique_ptr<Node> left = nullptr;
std::unique_ptr<Node> right = nullptr;
T key;
Node(T key) : key(std::move(key)) {}
};
public:
BinarySearchTree(std::initializer_list<T> init);
You should notice that:
- It uses
std::unique_ptr
because there is simply no excuse now! If you, for whatever reason, need something likeNode*
then it should rather beiterator
or node handle (C++17). - It has constructor accepting std::initializer_list to enable construction like
BinarySearchTree<int>{1,2,3}
- No
parent
inside node. I will elaborate this a bit more:
I can understand why you did this, because I have done it during my first exam and it was not received well by the teacher. You can use stacks tracking the path from root to current node (maybe make it part of the iterator if needed) to implement every exposed operation. I know it is easier to have the parent
in the node, but it should not be there.
Balancing the tree
You have provided constructor (BinarySearchTree(Node* nodes, unsigned n)
using build_bst(BinarySearchTree& tree, Node* nodes, unsigned n)
) that will produce well-balanced tree if the input array is sorted (should probably check and sort if needed), but did not make the tree read-only, which means, that you should provide some means of rebalancing the tree. Try reading about Red-Black Tree (and maybe AVL Tree as alternative).
Personal story: The purpose of the exam was to show understanding of pointers to handle joining two trees or heaps. The teacher could not imagine that somebody would even think about optimal solution... ehm, I did, and received some unwanted attention for the rest of my studies for doing so while helping myself using that parent
pointer instead of three stacks - there was simply not enough time to pull this out... and I was not supposed to even think about such solution, you know :D I hope you now understand that I am not stressing the removal of parent
too much :)
edited Dec 16 at 15:53
albert
1372
1372
answered Sep 28 at 17:34
firda
2,852627
2,852627
Thanks for the response. This was for my data structures class and we were expected to have the parent pointer. I have modified this code further and I could try implementing without the parent pointer
– Brady Dean
Sep 28 at 19:43
If the only purpose of the review was to check the code before you submit it to the teacher, then, well, it looks good for the level I am assuming you are at. You can testinsert
andremove
a bit more, to be sure, but I did not spot any obvious flaw in the implementation.
– firda
Sep 28 at 20:12
In your personal story, the parent pointer implementation was the optimal solution or wasn’t? I can see how it saves memory. I’m also implementing an avl right now
– Brady Dean
Sep 28 at 21:04
@BradyDean: Theparent
pointer did not change the speed, stack would produce same Big-O (some logarithm in it probably). I have used the pointer to even be able to write it ON PAPER IN TIME. The teacher expected something less-optimal (maybe quadratic complexity) but much easier to write (just tear the nodes from one tree/heap one at a time and put them in the bigger one). I was the only one to even try something smarter/faster. But adding that pointer to the node to make it easier to implement just made the teacher, ehm, mad at me.
– firda
Sep 28 at 21:11
Our professor haven’t even mentioned implementations without the parent pointer and hasn’t talked about using a stack when iterating through. I like the idea tho
– Brady Dean
Sep 28 at 21:14
|
show 1 more comment
Thanks for the response. This was for my data structures class and we were expected to have the parent pointer. I have modified this code further and I could try implementing without the parent pointer
– Brady Dean
Sep 28 at 19:43
If the only purpose of the review was to check the code before you submit it to the teacher, then, well, it looks good for the level I am assuming you are at. You can testinsert
andremove
a bit more, to be sure, but I did not spot any obvious flaw in the implementation.
– firda
Sep 28 at 20:12
In your personal story, the parent pointer implementation was the optimal solution or wasn’t? I can see how it saves memory. I’m also implementing an avl right now
– Brady Dean
Sep 28 at 21:04
@BradyDean: Theparent
pointer did not change the speed, stack would produce same Big-O (some logarithm in it probably). I have used the pointer to even be able to write it ON PAPER IN TIME. The teacher expected something less-optimal (maybe quadratic complexity) but much easier to write (just tear the nodes from one tree/heap one at a time and put them in the bigger one). I was the only one to even try something smarter/faster. But adding that pointer to the node to make it easier to implement just made the teacher, ehm, mad at me.
– firda
Sep 28 at 21:11
Our professor haven’t even mentioned implementations without the parent pointer and hasn’t talked about using a stack when iterating through. I like the idea tho
– Brady Dean
Sep 28 at 21:14
Thanks for the response. This was for my data structures class and we were expected to have the parent pointer. I have modified this code further and I could try implementing without the parent pointer
– Brady Dean
Sep 28 at 19:43
Thanks for the response. This was for my data structures class and we were expected to have the parent pointer. I have modified this code further and I could try implementing without the parent pointer
– Brady Dean
Sep 28 at 19:43
If the only purpose of the review was to check the code before you submit it to the teacher, then, well, it looks good for the level I am assuming you are at. You can test
insert
and remove
a bit more, to be sure, but I did not spot any obvious flaw in the implementation.– firda
Sep 28 at 20:12
If the only purpose of the review was to check the code before you submit it to the teacher, then, well, it looks good for the level I am assuming you are at. You can test
insert
and remove
a bit more, to be sure, but I did not spot any obvious flaw in the implementation.– firda
Sep 28 at 20:12
In your personal story, the parent pointer implementation was the optimal solution or wasn’t? I can see how it saves memory. I’m also implementing an avl right now
– Brady Dean
Sep 28 at 21:04
In your personal story, the parent pointer implementation was the optimal solution or wasn’t? I can see how it saves memory. I’m also implementing an avl right now
– Brady Dean
Sep 28 at 21:04
@BradyDean: The
parent
pointer did not change the speed, stack would produce same Big-O (some logarithm in it probably). I have used the pointer to even be able to write it ON PAPER IN TIME. The teacher expected something less-optimal (maybe quadratic complexity) but much easier to write (just tear the nodes from one tree/heap one at a time and put them in the bigger one). I was the only one to even try something smarter/faster. But adding that pointer to the node to make it easier to implement just made the teacher, ehm, mad at me.– firda
Sep 28 at 21:11
@BradyDean: The
parent
pointer did not change the speed, stack would produce same Big-O (some logarithm in it probably). I have used the pointer to even be able to write it ON PAPER IN TIME. The teacher expected something less-optimal (maybe quadratic complexity) but much easier to write (just tear the nodes from one tree/heap one at a time and put them in the bigger one). I was the only one to even try something smarter/faster. But adding that pointer to the node to make it easier to implement just made the teacher, ehm, mad at me.– firda
Sep 28 at 21:11
Our professor haven’t even mentioned implementations without the parent pointer and hasn’t talked about using a stack when iterating through. I like the idea tho
– Brady Dean
Sep 28 at 21:14
Our professor haven’t even mentioned implementations without the parent pointer and hasn’t talked about using a stack when iterating through. I like the idea tho
– Brady Dean
Sep 28 at 21:14
|
show 1 more comment
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f204378%2fbinary-search-tree-with-common-operations%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown