c++ Binary Search tree Deletion

Question

So, my question is I don't understand why this doesn't work. I commented below where it is saying that parent is never initialized when it clearly is. Am I doing pointers wrong, am I getting the logic backwards am I so far off it would be better to just start from scratch? This is the most difficult assignment I have encountered so any help at all would be very beneficial.

void Dictionary::remove(string word)
{
if(root == NULL)
{
    cout << "list is empty\n";
    return;
}
DictionaryNode *curr = root;
DictionaryNode *parent = NULL;`

while(curr != NULL)
{
    if(curr->word == word)
        break;
    else
    {
        parent = curr;
        if(word > curr->word)
            curr = curr->right;
        else
            curr = curr->left;
    }
}
//LEAF node.
if(curr->left == NULL && curr->right == NULL)
{
    if(parent->left == curr) // Right here is an access violation. Which doesn't //make sense.
    {
        parent->left = NULL;
    }
    else
    {
        parent->right = NULL;
    }
    delete curr;
}

/*
* Node has a single child LEFT or RIGHT
*/  
if((curr->left == NULL && curr->right != NULL) || (curr->left != NULL && curr->right == NULL))
{
    if(curr->left == NULL && curr->right != NULL)
    {
        if(parent->left == curr) //if(parent->left == curr) //says parent is //not intialized
        {
            parent->left = curr->right;
            delete curr;
        }
        else
        {
            parent->right = curr->right;
            delete curr;
        }
    }

    else
    {
        if(parent->left == curr)
        {
            parent->left = curr->left;
            delete curr;
        }
        else
        {
            parent->right = curr->left;
            delete curr;
        }
    }

}

 if (curr->left != NULL && curr->right != NULL)
{
    DictionaryNode* temp; 
    if(parent == NULL || parent->left==curr)
    {  
        temp = curr->right;
        while(temp->left!=NULL)
            temp = temp->left;
        if(parent!=NULL)
            parent->left = curr->right;
        else
            root = curr->right;
        temp->left = curr->left;
        curr->left = curr->right=NULL;
        delete curr;

    } 

    else if(parent->right==curr)
    {
        temp = curr->left;
        while(temp->right!=NULL)
            temp = temp->right;
        parent->right=curr->left;
        temp->right = curr->right;
        curr->left = curr->right=NULL;
        delete curr;
    }
  }

}

What if the dictionary contains exactly 1 element? `curr == root`, `parent == NULL`, `parent->left` is an access violation. — timrau, Mar 28 '14 at 00:15
Also, you tried to access `curr->left` after `delete curr;`. This is obviously a freed memory read. — timrau, Mar 28 '14 at 00:17
First off, thank you for editing it I couldn't for the life of me figure that out. If i had one element, it works. I should have been more specific in my question. It's when i go to delete something that it starts to throw errors at my face. — varrick, Mar 28 '14 at 00:18

score 0 · Answer 1 · answered Mar 28 '14 at 00:43

1. First thing I see:

while(curr != NULL)
{
 //stuff
}

As it is written, it seems that at the end of your loop curr == NULL

Lazy me had to look at the content of your loop to notice the break. A break could be even less noticeable with a bigger block in the loop. This is not a good practice.

Use a bool (e.g.: bool isNodeFound;), it's cheap (one bit) and makes it more clear. while(curr != NULL && !isNodeFound) is more clear of your intentions, at first sight, without looking at the content of your loop.

2.What if indeed you don't hit the break in the loop and curr == NULL ? Your next instruction curr->left would fail! Seems like the Boolean will be useful again!

if(!isNodeFound)
{
//log error if you can "cannot remove node because it is not in dictionary"
return false; //make your function a bool to return if it succeeded or not
}

Try to analyze the rest of your code with the same state of mind, more clarity and testing, let me know if it works.

Well see I understand what you mean but, I don't think making the function boolean will really help me. The thing is, it deletes some of the stuff whether it's the root or the last node or the last child. It's just sometimes, it gives me access violation. I don't understand it but i'll try what you say. Thank you — varrick, Mar 28 '14 at 00:52
technically a bool uses a byte of space and not a bit, as the computer can't address individual bits, only bytes. — isaac9A, Aug 29 '15 at 21:37
std::vector can pack bools into bits! but yeah I meant byte haha, thanks varrick — vincentB, Sep 07 '15 at 18:01

score 0 · Answer 2 · answered Oct 16 '18 at 10:40

everyone. One day, I searched this question when i needed function to remove tree node in BST. So, this question is nice, i edited and checked above code then code really operated successfully. Above code missed some instances, follow me below explanations:

First, deleted node is LEAF NODE. You missed a instance that node is either root or leaf node (i.e. BST only have a node). So, parent is NULL and parent->left/right is invalid.

Second, deleted node has a subtree left or right. So, this is similar with First if deleted node is root.

Third, deleted node have left and righr subtree. You considered "parent" but you shouldn't use "if(parent == NULL || parent->left==curr)" as if parent = NULL so that parent->left is invalid. You should make " if(parent == NULL){...} else{if(parent->left == curr)...}".

Finally, use if...else-if...else instead of using if...if...if because you deleted "curr", then you won't know "curr" point anywhere and "if" next still will be checked with "curr" errors.

Below edited code for anyone need,

void Dictionary::remove(string word)
{
    if(root == NULL)
    {
        cout << "list is empty\n";
        return;
    }
    DictionaryNode *curr = root;
    DictionaryNode *parent = NULL;

    while(curr != NULL)
    {
        if(curr->word == word)
            break;
        else
        {
            parent = curr;
            if(word > curr->word)
                curr = curr->right;
            else
                curr = curr->left;
        }
    }
    //LEAF node.
    if(curr->left == NULL && curr->right == NULL)
    {
        if (parent == NULL) {
            delete curr;
        } else {
            if(parent->left == curr) // Right here is an access violation. Which doesn't //make sense.
            {
                parent->left = NULL;
            }
            else
            {
                parent->right = NULL;
            }
            delete curr;
        }
    }

    /*
    * Node has a single child LEFT or RIGHT
    */  
    else if((curr->left == NULL && curr->right != NULL) || (curr->left != NULL && curr->right == NULL))
    {
        if(curr->left == NULL && curr->right != NULL)
        {
            if (parent == NULL) {
                    root = curr->right;
                    curr->right = NULL;
                    delete curr;
            } else {
                if(parent->left == curr) //if(parent->left == curr) //says parent is //not intialized
                {
                    parent->left = curr->right;
                    delete curr;
                }
                else
                {
                    parent->right = curr->right;
                    delete curr;
                }
            }
        }

        else
        {
            if (parent == NULL) {
                    root = curr->left;
                    curr->left = NULL;
                    delete curr;
            } else {
                if(parent->left == curr)
                {
                    parent->left = curr->left;
                    delete curr;
                }
                else
                {
                    parent->right = curr->left;
                    delete curr;
                }
            }
        }

    }
    else
    {
        DictionaryNode* temp; 
        if(parent == NULL)
        {  
            temp = curr->right;
            while(temp->left!=NULL)
                temp = temp->left;
            if(parent!=NULL)
                parent->left = curr->right;
            else
                root = curr->right;
            temp->left = curr->left;
            curr->left = curr->right=NULL;
            delete curr;

        } else {
            if(parent->left==curr){
                temp = curr->right;
                while(temp->left!=NULL)
                    temp = temp->left;
                if(parent!=NULL)
                    parent->left = curr->right;
                else
                    root = curr->right;
                temp->left = curr->left;
                curr->left = curr->right=NULL;
                delete curr;
            }
            else if(parent->right==curr)
            {
                temp = curr->left;
                while(temp->right!=NULL)
                    temp = temp->right;
                parent->right=curr->left;
                temp->right = curr->right;
                curr->left = curr->right=NULL;
                delete curr;
            }
        }
    }
}

Hope this code that can help other people when they need!

c++ Binary Search tree Deletion

2 Answers2