Broke the binary tree, please fix :-)

test/binarytree.cpp

@@ -1,22 +1,22 @@
 #include "../tree/binarytree.hpp"
 #include "testutil.hpp"
-#include <unordered_set>
+#include <vector>
 
 void FillWithRandomNumbers(BinaryTree::Tree<unsigned> & tree,
-	std::unordered_set<unsigned> & control,
+	std::vector<unsigned> & control,
 	unsigned const count = 512u)
 {
 	for(unsigned i = 0u; i < count; ++i)
 	{
 		unsigned const value = Util::GetRandomNumber();
-		control.insert(value);
+		control.push_back(value);
 		tree.Insert(value);
 	}
 }
 
 bool TestInsertContains()
 {
-	std::unordered_set<unsigned> control;
+	std::vector<unsigned> control;
 	BinaryTree::Tree<unsigned> tree;
 
 	FillWithRandomNumbers(tree, control);
@@ -26,7 +26,7 @@ bool TestInsertContains()
 	{
 		if(!tree.Contains(num))
 		{
-			std::printf("Value %u inserted but cannot be found!\n", num);
+			std::printf("\tValue %u inserted but cannot be found!\n", num);
 			good = false;
 		}
 	}
@@ -36,23 +36,19 @@ bool TestInsertContains()
 
 bool TestDeletion()
 {
-	std::unordered_set<unsigned> control;
+	std::vector<unsigned> control;
 	BinaryTree::Tree<unsigned> tree;
 
 	FillWithRandomNumbers(tree, control);
 
-	unsigned i = 0u;
 	unsigned const toDeleteCount = control.size() / 4;
-	while(i < toDeleteCount)
+	for(unsigned i = 0u; i < toDeleteCount; ++i)
 	{
-		auto const toDelete = Util::GetRandomNumber();
+		auto const toDeleteIndex = Util::GetRandomNumber() % control.size();
-		auto iter = control.find(toDelete);
+		auto const toDeleteValue = control[toDeleteIndex];
-		if(iter != control.end())
+
-		{
+		control.erase(control.begin() + toDeleteIndex);
-			control.erase(iter);
+		tree.Delete(toDeleteValue);
-			tree.Delete(i);
-			++i;
-		}
 	}
 	
 	bool good = true;

test/testutil.hpp

@@ -48,11 +48,11 @@ void Execute(bool (*testFunction)(std::vector<std::string> &), char const * cons
 		}
 		else
 		{
+			std::printf("[FAIL] %s\n", message);
 			for(auto & issue : issues)
 			{
 				std::printf("      Issue: %s\n", issue.c_str());
 			}
-			std::printf("[FAIL] %s\n", message);
 		}
 	}
 	catch(std::exception & e)

tree/binarytree.hpp

@@ -6,8 +6,6 @@ namespace BinaryTree
 template <class T>
 struct Node
 {
-	Node *parent;
-
 	T value;
 
 	std::unique_ptr<Node<T>> left;
@@ -18,9 +16,10 @@ struct Node
 		return left == nullptr && right == nullptr;
 	}
 
-	Node(T const &_value, Node<T> *_parent)
+	Node(T const &_value)
-			: parent(_parent),
+			: value(_value),
-				value(_value)
+			left(nullptr),
+			right(nullptr)
 	{
 	}
 };
@@ -31,9 +30,9 @@ class Tree
 private:
 	std::unique_ptr<Node<T>> root;
 
-	std::unique_ptr<Node<T>> & GetRightMostLeaf(std::unique_ptr<Node<T>> &localRoot)
+	std::unique_ptr<Node<T>> & GetRightMostLeaf(std::unique_ptr<Node<T>> &node)
 	{
-		std::unique_ptr<Node<T>> *nodePtr = &localRoot;
+		std::unique_ptr<Node<T>> *nodePtr = &node;
 		while ((*nodePtr)->right != nullptr)
 		{
 			nodePtr = &((*nodePtr)->right);
@@ -42,91 +41,91 @@ private:
 		return *nodePtr;
 	}
 
+	void DeleteNode(std::unique_ptr<Node<T>> & node)
+	{
+		if(node == nullptr || node->IsLeaf())
+		{
+			node.release();
+		}
+
+		if(node->left == nullptr)
+		{
+			node = std::move(node->right);
+		}
+
+		if(node->right == nullptr)
+		{
+			node = std::move(node->left);
+		}
+
+		auto & replacementNode = GetRightMostLeaf(node);
+		replacementNode->left.swap(node->left);
+		replacementNode->right.swap(node->right);
+		node.swap(replacementNode);
+
+		replacementNode.release();
+	}
+
+	std::unique_ptr<Node<T>> & FindNode(T const & value)
+	{
+		std::unique_ptr<Node<T>> *currentPtr = &root;
+		while (*currentPtr != nullptr && (*currentPtr)->value != value)
+		{
+			if ((*currentPtr)->value > value)
+			{
+				currentPtr = &((*currentPtr)->left);
+			}
+			else
+			{
+				currentPtr = &((*currentPtr)->right);
+			}
+		}
+
+		return *currentPtr;
+	}
+
 public:
-	void Insert(T value)
+	void Insert(T const & value)
 	{
-		Node<T> *previousPtr = nullptr;
+		std::unique_ptr<Node<T>> *currentPtr = &root;
-		std::unique_ptr<Node<T>> *rootPtr = &root;
+		while ((*currentPtr) != nullptr)
-		while (*rootPtr != nullptr)
 		{
-			previousPtr = rootPtr->get();
+			if ((*currentPtr)->value > value)
-			if ((*rootPtr)->value > value)
 			{
-				rootPtr = &((*rootPtr)->left);
+				currentPtr = &((*currentPtr)->left);
 			}
 			else
 			{
-				rootPtr = &((*rootPtr)->right);
+				currentPtr = &((*currentPtr)->right);
 			}
 		}
-		*rootPtr = std::make_unique<Node<T>>(value, previousPtr);
+		*currentPtr = std::make_unique<Node<T>>(value);
 	}
 
-	bool Contains(T const value)
+	void InsertNoDuplicates(T const & value)
 	{
-		std::unique_ptr<Node<T>> *rootPtr = &root;
+		auto & toInsertIn = FindNode(value);
-		while (*rootPtr != nullptr)
+		if(toInsertIn != nullptr)
 		{
-			if ((*rootPtr)->value == value)
+			return;
-			{
-				return true;
-			}
-
-			if ((*rootPtr)->value > value)
-			{
-				rootPtr = &((*rootPtr)->left);
-			}
-			else
-			{
-				rootPtr = &((*rootPtr)->right);
-			}
 		}
 
-		return false;
+		toInsertIn = std::make_unique<Node<T>>(value);
 	}
 
-	void Delete(T const value)
+	bool Contains(T const & value)
 	{
-		std::unique_ptr<Node<T>> *rootPtr = &root;
+		return FindNode(value) != nullptr;
-		while (*rootPtr != nullptr)
+	}
+
+	void Delete(T const & value)
+	{
+		auto & node = FindNode(value);
+		if (node == nullptr)
 		{
-			if ((*rootPtr)->value == value)
+			return;
-			{
-				if ((*rootPtr)->IsLeaf())
-				{
-					rootPtr->release();
-					return;
-				}
-
-				if ((*rootPtr)->left == nullptr)
-				{
-					rootPtr->swap((*rootPtr)->right);
-					(*rootPtr)->right.release();
-				}
-				else if ((*rootPtr)->right == nullptr)
-				{
-					rootPtr->swap((*rootPtr)->left);
-					(*rootPtr)->left.release();
-				}
-				else
-				{
-					auto &replacementNode = GetRightMostLeaf((*rootPtr)->left);
-					rootPtr->swap(replacementNode);
-					replacementNode.release();
-				}
-
-				return;
-			}
-
-			if ((*rootPtr)->value > value)
-			{
-				rootPtr = &((*rootPtr)->left);
-			}
-			else
-			{
-				rootPtr = &((*rootPtr)->right);
-			}
 		}
+		DeleteNode(node);
 	}
 
 	Tree()