List All Function still trying to retrieve a node

2019-09-09 19:22发布

问题:

I have these functions to remove a node from my binary search tree:

bool collection::removeFromTree(const char name[])
{
    for (treeNode * curr = root; curr;)
    {
        int8_t result = strcmp(name, curr->item->getName());
        if (result == 0)
        {
            deleteNode(curr);
            return true;
        }
        else if (result < 0)
            curr = curr->left;
        else if (result > 0)
            curr = curr->right;
    }
    return false;
}
void collection::deleteNode(treeNode *& goneNode)
{
    //if it's a leaf
    if (!goneNode->left && !goneNode->right)
    {
        delete goneNode; //node's destructor gets invoked
        goneNode = nullptr;
    }
    //if it has right child
    else if (!goneNode->left)
    {
        goneNode = goneNode->right;
    }
    //if it has left child
    else if (!goneNode->right)
    {
        goneNode = goneNode->left;
    }
    //if it has both children
    else
    {
        treeNode * prev = nullptr;
        treeNode * curr = goneNode->right;
        while (curr->left)
        {
            prev = curr;
            curr = curr->left;
        }
        //prev points to the copy over data
        delete goneNode->item;
        if (!prev)
        {
            goneNode->item = curr->item;
            goneNode->right = curr->right;
            curr->item = nullptr;
        }
        else
        {
            goneNode->item = curr->item;
            curr->item = nullptr;
            prev->left = curr->right;
        }
    }
}

This runs fine, but when I try to list all the elements in my tree after deleting a node (with these functions):

void collection::displayByName() const
{
    std::cout << std::endl
        << "========================================" << std::endl;
    //display the tree inorder
    listAll(root);
}
void collection::listAll(const treeNode * const & root) const
{
    if (root)
    {
        std::cout << *(root->item) << std::endl
            << "========================================" << std::endl;
        listAll(root->left);
        listAll(root->right);
    }
}

I receive this error:

And when I quit the program after deleting a node (invoking these destructors):

collection::~collection()
{
    delete root;
}
collection::treeNode::~treeNode()
{
    delete left;
    delete right;
}

I recieve this error:

Any suggestions would be greatly appreciated because I see no reason for my listAll() function to be calling nodes that I've already deleted.

By the way, this is my struct for my treeNode:

struct treeNode
    {
        treeNode();
        treeNode(vendor *& item);
        ~treeNode();
        vendor * item;
        treeNode *left, *right;
    };
    treeNode * root;    //the bst

    hashNode ** table;  //the hash table
    uint8_t capacity;
    uint8_t size;
    const static uint8_t INIT_CAP = 20;

回答1:

When you need to remove a node from a singly linked list or a tree, I find using a pointer to pointer is handy. Namely, if we have a treeNode** ptr;, then *ptr is the pointer to our node. So, if ptr = &root, then *ptr = nullptr sets root to nullptr.

I removed the deleteNode function and threw its logic in the removeFromTree function.

bool collection::removeFromTree(const char name[])
{
    treeNode** ptr = &root;

Instead of being a pointer to treeNode, ptr will point to a treeNode* inside the tree structure. This way, we can modify the pointer that led us to the current node. The lines marked //same as before have the same logic you were using, just possibly modified to account for the fact ptr has another level of dereferencing to do.

    int result; //same as before
    while (*ptr) //While we haven't hit a dead end
    {
        result = strcmp(name, (*ptr)->item->getName()); //same as before
        if (result < 0) //same as before
            ptr = &((*ptr)->left); //same as before
        else if (result > 0) //same as before
            ptr = &((*ptr)->right); //same as before
        else //begin deleteNode() logic
        {
            if ((*ptr)->left && (*ptr)->right) //two children
            {

Here, we use pointers to member because the alternative was a conditional operator on every line. If a node has two children, we need to find either the rightmost node on the left side, or the leftmost node on the right side. That's the node we can replace the current node with.

                treeNode* treeNode::*dir = some_condition ? &treeNode::right : &treeNode::left; //pointer to treeNode member of type treeNode*
                treeNode* treeNode::*ndir = some_condition ? &treeNode::left : &treeNode::right;  //pointer to treeNode member of type treeNode*

dir now either points to left or right, which is the direction we are searching the tree for. ndir is the opposite direction. So, if we want the rightmost node on the left side, (*ptr)->*dir == (*ptr)->left and (*ptr->*ndir == (*ptr)->right. If we want the leftmost right node, it would be reversed. This is just a more complicated way to do less work, really. It shouldn't be hard to remove. some_condition is just either true or false. true means the left side of the tree (from the current node) loses a node, and false means the right side does.

                treeNode** replacement = &((*ptr)->*ndir); //the node to replace the current one with
                while ((*replacement)->*dir) //While we aren't at the edge
                    replacement = &((*replacement)->*dir);

This loops until *replacement is the node we need to replace *ptr with.

                treeNode* rep_branch = (*replacement)->*ndir; //If the replacement node had a child, this is now it

                (*replacement)->left = (*ptr)->left; //Copy current to replacement
                (*replacement)->right = (*ptr)->right; //Copy current to replacement
                (*ptr)->left = nullptr; //null out current in case of destructor
                (*ptr)->right = nullptr; //null out current in case of destructor

Now, the replacement node is pointing to the node-to-be-deleted's children, and our soon to be expired node has no children anymore. Now, it's safe to delete the unwanted node. If the node class had a destructor to delete its children, the left and right pointers were set to nullptr just in case.

                delete *ptr; //delete unwanted node
                *ptr = *replacement; //replacement node has taken the unwanted node's place in the tree
                *replacement = rep_branch; //The replacement's child takes its own place
            }

This completes the tree's structure. Wherever the unwanted node was, the replacement node has taken its place. And because the replacement node was required to be an edge node, it had at most one child. We just replace it with the child.

            else if ((*ptr)->left) //one child on left
            {
                treeNode* current = *ptr;
                *ptr = (*ptr)->left; //replace current with left
                current->left = nullptr; //null out for safety
                delete current;
            }
            else if ((*ptr)->right) //one child on right
            {
                treeNode* current = *ptr;
                *ptr = (*ptr)->right; //replace current with right
                current->right = nullptr; //null out for safety
                delete current;
            }
            else //no children
            {
                delete *ptr;
                *ptr = nullptr;
            }
            return true; //yay it's over
        }
    }
    return false; //never found it
}

The rest is fairly straightforward, just replacing easier nodes and returning. Hopefully this gives you some ideas about how to approach problems like this, and the occasional uses of some of these structures. This is what I meant about using treeNode** over treeNode* for operations like this.