Iterative BST insertion in C++

Question

I am trying to understand BSTs and how to insert elements into it iteratively. My node structure implementation looks like so:

struct Node{
    Node *left;
    Node *right;
    T data; //template class   
};

And my insertion implementation looks like so:

template<typename T>
bool BST<T>::Insert(const T value)
{
   Node *newNode = new Node;

   newNode -> data = value;
   newNode -> left = NULL; 
   newNode -> right = NULL;

   if(root == NULL) {root = newNode;} //If the BST is empty
   else 
   {//The BST is not empty 
      Node *ptr = root; //points to the current Node
      Node *ptr_parent; //points to the parent Node

      while(ptr != NULL)
      {
         if((ptr -> data) > value)
         {   
            ptr_parent = ptr;    
            ptr = ptr -> left;
         }

         if((ptr -> data) < value)
         {
            ptr_parent = ptr;
            ptr = ptr -> right;
         }
      }
    }

      ptr = newNode; //insert the newNode at the spot
      if((ptr_parent -> data) < value)
         ptr_parent -> right = newNode;
      else
         ptr_parent -> left = newNode; 

   return true;
}

The insertion works when adding the first Node into an empty tree but I get a segmentation fault whenever i try to add more Nodes. I understand that there are posts that show how to implement insertions into BSTs but most of them show the recursive method, and those with iterative examples are incomplete or too specific. Thank you.

Answer 1

I think I'd do things a little differently. First, I'd simplify the other code a little by adding a ctor to the Node class:

struct Node{
    Node *left;
    Node *right;
    T data; 

    Node(T const &data) : left(nullptr), right(nullptr), data(data) {}
};

Then you can use a pointer to a pointer to traverse the tree and insert the item:

bool insert(const T value) {
    Node **pos;
    for (pos = &root; *pos != nullptr;) {
        if (value < (*pos)->value) 
            pos = &(*pos)->left;
        else if ((*pos)->value < value ) 
            pos = &(*pos)->right;
        else 
            return false;
    }
    *pos = new Node(value);
    return true;
}

Note that I've delayed creating the new node until after we've dropped out of the loop. This way, if we have a duplicate element, we can just return (without leaking a node, since we haven't allocated a new node yet).

For what it's worth, if you were going to do this recursively, it would probably be easier to use a reference to a pointer instead of a pointer to a pointer.