Python: BST removal function deleting multiple nodes and reattaching duplicates

Question

I have 2 functions to remove nodes from a binary search tree. The first is to remove the root of the tree, and the second is to remove any other node in the tree.

The issue is when testing after the 3rd iteration, things start to get wonky. Line for DEL: 45 deletes nodes 45, 30, 20, and the line for DEL: 40 does not delete 40 but deletes everything after 40 and repeatedly reattaches 30, 20, 45.

I have the feeling something is wrong with the while loop and whatever that issue is, is making its way down into the next set of loops and breaking the tree.

The expected results should remove only the value described and keep the tree structure as close to the same as it was originally. What am I doing wrong with the reattachment of existing nodes? When I am debugging, I have found that

        if node.right is None and node.left is None:
            pn.right = None
            pn.left = None

Is what is removing the nodes 20 and 30 from the DEL 45 line

    def remove_start_node(self) -> bool:
        """
        deletes the root note of the BST. first checks if the BST is empty and if there
        is only the root exists. If empty, return False. If only the root exists, delete the root node.
        else, find the in order successor of the root node(leftmost child of the right subtree.)
        if the deleted node only has a left subtree,the left node becomes the rood node of the subtree.
        """

        if self._root is None:
            return False
        if self._root.left is None and self._root.right is None:
            self._root = None
        elif self._root.right is None:  # checks if only left subtree exists
            self._root = self._root.left
        else:
            subtree = self._root.right
            par_tree = subtree
            while subtree.left is not None:  # traverse down till the in order successor is found (leftmost child)
                par_tree = subtree
                subtree = subtree.left
            if subtree != self._root.right:  # reestablish structure
                par_tree.left = subtree.right
                subtree.right = self._root.right
            subtree.left = self._root.left
            self._root = subtree
        return True

    def remove(self, value) -> bool:
        """
        first traverses throughout the BST and deletes the target value while restructuring the BST.
        # first checks if BST is empty, if there is only one node, and if the value is contained within the BST.
        # if empty, return False. if only node, delete the root node. else, find the in order successor of the current
        # node which is the leftmost child of the right subtree of the current node. If the deleted node only has the
        # left subtree, the current node becomes the rood node of the left subtree.
        """

        if not self.contains(value):  # check if the value exists
            return False
        if self._root is None:  # checks if BST is empty
            return False
        if self._root.value == value:  # checks if the value matches the root node
            self.remove_start_node()
            return True

        # traverse through the tree first until the value is found
        x = self._root
        pn = None
        while x is not None:  # traverse through the tree
            if x.value == value:
                node = x
                break
            elif value < x.value:
                pn = x
                x = x.left
            else:
                pn = x
                x = x.right

        # if successor has no children, parent node's children is updated to None
        if node.right is None and node.left is None:
            pn.right = None
            pn.left = None
        elif node.right is None:  # if successor only has a left child, point parent to its children
            pn.right = node.left
        else:  # once successor is found, traverse to the left most child
            subtree = node.right
            par_tree = subtree
            while subtree.left is not None:
                par_tree = subtree
                subtree = subtree.left
            if subtree != node.right:  # reestablish structure
                par_tree.left = subtree.right
                subtree.right = node.right
            pn.right = subtree  # point parent to new subtree
            temp = node.left  # store any other subtrees from the deleted node
            node = subtree  # replace successor with current node
            node.left = temp  # reattach remaining subtrees
        return True

-------------------------------
INPUT  : BST pre-order { 1, 2, 3 } DEL: 1
RESULT : BST pre-order { 2, 3 }
INPUT  : BST pre-order { 1, 2, 3 } DEL: 2
RESULT : BST pre-order { 1, 3 }
INPUT  : BST pre-order { 1, 2, 3 } DEL: 3
RESULT : BST pre-order { 1, 2 }
INPUT  : BST pre-order { 50, 40, 30, 20, 45, 60, 70, 80 } DEL: 0
RESULT : BST pre-order { 50, 40, 30, 20, 45, 60, 70, 80 }
**INPUT  : BST pre-order { 50, 40, 30, 20, 45, 60, 70, 80 } DEL: 45
RESULT : BST pre-order { 50, 40, 60, 70, 80 }
INPUT  : BST pre-order { 50, 40, 30, 20, 45, 60, 70, 80 } DEL: 40
RESULT : BST pre-order { 50, 40, 30, 20, 45, 30, 20, 45, 30, 20 }**
INPUT  : BST pre-order { 50, 40, 30, 20, 45, 60, 70, 80 } DEL: 30
RESULT : BST pre-order { 50, 40, 30, 20, 20, 60, 70, 80 }

Answer 1

Some of the issues are:

• In the "easy" cases you are assuming that the last descent came by choosing the right child, as you set pn.right to something, but it might well be that you came via the left child, and then you should set pn.left.

• par_tree is not initialised to the correct node. To be consistent, it should be set to the parent of subtree, i.e. to node.

• The part that "reestablishes the structure" was unclear to me. The comments are not reflecting what is happening. For instance, node = subtree is not going to "replace successor with current node"... it merely sets a name. But more importantly. The common algorithm would be to take the value of subtree and store it in node and then remove subtree from the tree (which is an "easy" case, as it does not have a left child).

Not a problem, but:

• it is not necessary to write a complete method for deleting the root. The actions are so similar, that you could deal with them in one method.

• it is not necessary to first call contains to see if the value exists. This is something that will appear naturally as you execute the removal algorithm.

Here is a proposed fix -- comments indicate the main changes

    def remove(self, value) -> bool:
        if self._root is None:
            return False
        x = self._root
        pn = None
        while x is not None:
            if x.value == value:
                node = x
                break
            elif value < x.value:
                pn = x
                x = x.left
            else:
                pn = x
                x = x.right
        else:
            return False  # not found
        # Deal with easy cases in one block of code
        if node.right is None or node.left is None:
            if not pn:  # Case where the root is deleted
                self._root = node.left or node.right
            # Be aware: node could be either the left or right child of its parent
            elif pn.left is node:
                pn.left = node.left or node.right
            else:
                pn.right = node.left or node.right
        else:
            par_tree = node
            subtree = node.right
            while subtree.left is not None:
                par_tree = subtree
                subtree = subtree.left
            # Don't delete node, but its value, and delete subtree-node instead 
            # Move the value of subtree
            node.value = subtree.value
            # Then remove that node (easy case)
            if par_tree.left == subtree:
                par_tree.left = subtree.right
            else:
                par_tree.right = subtree.right
        return True