0

So I'm working on a BST, building the delete tools.

My code sequence seems to work right - Save not updating the parent or root and setting the pointer that sent it down to the deleted Node's address to NULL.

I'm passing a pointer to a pointer in my Erase and RemoveNode functions, so as to directly effect the left, right, or root data members that actually lead to the recursive call. In walking through the code, it sets *N to NULL in the remove function, but this is not reflected in the data of the calling object. Am I incorrect in using the pointer-of-a-pointer method? If so, is there a way I can recursively delete and be able to modify the prior node if the link is destroyed?

Node struct:

struct tNode
{
    tNode(int n)
    {
        data = n; 
        left = NULL; 
        right = NULL;
    }

    //Works, cleans all linked objects. 
    //Must remember to null links when removing wanted nodes
    ~tNode(void)
    {
        //cout << "Deleting " << data << endl;
        delete left;
        delete right;
    }

    // Data members
    int data;
    tNode* left;
    tNode* right;
};

Eraser function to recurse over the tree:

    void BinSearchTree::Erase(int n, tNode** N)
    {
        tNode* node = *N;
        if (root)
        {
            if (node->data > n) // post order, to avoid moving many times over
            {
                if (node->left)
                {
                    Erase(n, &node->left);
                }
            }
            else
            {
                if (node->right)
                {
                    Erase(n, &node->right);
                }
            }

            if (node->data == n)
            {
                RemoveNode(&node);
            }
        }
    }

And the RemoveNode function to handle actual deletion:

void BinSearchTree::RemoveNode(tNode** N)
{
    tNode* node = *N;
    if (!node->left && !node->right) // is leaf
    {
        delete node; // remove node
        size--;
        *N = NULL; // null pointer for above node/structure
    }
    else if (!node->left) // right child
    {
        tNode* temp = node->right; // to strip out copied node when finished
        node->data = node->right->data; // copy right node into current node
        node->right = node->right->right;
        node->left = node->right->left;

        temp->right = NULL; // NULL because node destructor is recursive
        temp->left = NULL; // ^^
        delete temp;
        size--;
    }
    else if (!node->right) // left child
    {
        tNode* temp = node->left; // to strip out copied node when finished
        node->data = node->left->data; // copy left node into current node
        node->right = node->left->right;
        node->left = node->left->left;

        temp->right = NULL; // NULL because node destructor is recursive
        temp->left = NULL; // ^^
        delete temp;
        size--;
    }
    else // 2 children
    {
        tNode* temp = node->right; // find ideal child -> left-most right child
        tNode* parent = NULL; // keep track of owner of ideal child
        while (temp->left)
        {
            parent = temp;
            temp = temp->left;
        }

        node->data = temp->data; // copy ideal child to root
        if (parent)
        {
            parent->left = temp->right; // case that left-most child has right child of it's own
        }
        RemoveNode(&temp);
        size--;
    }
}
2
  • Do you have an actual question? Commented Nov 7, 2011 at 1:11
  • @KerrekSB Sorry, I must have gotten ahead of myself. Thanks for pointing it out. Commented Nov 7, 2011 at 1:20

2 Answers 2

1

I think I see it. When you call RemoveNode() from Erase(), you pass it the value &node. Pass in N instead.

Here's what's happening: The line tNode* node = *N; creates a local variable on the stack. A copy of *N. And while that variable has the same value as *N (at first), it's stored in a different place in memory: node is on the stack, and *N is somewhere in the heap.

So, since you pass &node to RemoveNode(), node is what gets changed. Not *N - it's somewhere else. But if you pass in, you're changing what you want to.

Hope that helps!

PS: If that's not clear, let me know! Writing about double-pointers might just be harder than using them...

Sign up to request clarification or add additional context in comments.

1 Comment

Good catch on the local scope, made that mistake a few places (wasn't thinking about it I guess). Thanks!
0

Save yourself the trouble of using double pointers and just use a reference to pointer. They have the same semantic of normal pointers, but assigning them will actually change the passed pointer.

void BinSearchTree::Erase(int item, tNode*& node);

Also, use expressive names and save the single letter variable names for loops.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.