Stuck with deleting node from tree

Question

I'm trying to build a max heap in VC++ using Visual Studio 2008 v9.0.30729.1 SP.

In the tree, each node looks like:

typedef struct node{
    struct data_t *data;
    struct node_t *left;
    struct node_t *right;
}node_t;

A single node creation logic goes like this:

node_t* createNode(int id, int pID, float probability)
{
    node_t *temp = (node_t *)malloc(sizeof(node_t));
    data_t *data = (data_t *)malloc(sizeof(data_t));

    data->id = id;
    data->pID = pID;
    data->probability = probability;

    temp->data = data;
    temp->left = 0;
    temp->right = 0;

    return temp;
}

I have managed to create and insert elements in the tree (insertion logic working fine). I'm stuck with the logic of removing a node (a leaf, to be precise) from this tree.

I've tried four different approaches for the same:

node_t* deleteLeaf(node_t* heap)
{
    node_t* leaf;


    if((heap->left==0) && (heap->right==0))
    {
        //heap = 0;     //APROACH 1
        //heap->data = 0;   //APROACH 2

        return heap;
    }
    else if((heap->left!=0) && (heap->right==0))
    {
        leaf = deleteLeaf(heap->left);

    }
    else 
    {
        leaf = deleteLeaf(heap->right);

    }

    //leaf = 0;         //APROACH 3
    //free(leaf);           //APROACH 4 

    return leaf;

}

(Uncomment APPROACH 1/2/3/4 for the desired effect).

None of this seems to work. I need to assign a zero/null value to the left/right pointer of the previous node.

How to make this work? Please help.

Answer 1

Instead of writing 4 random lines of code and calling them "approaches", try actually specifying a function that does something meaningful. A function that takes a heap as an argument should be called DeleteHeap, not DeleteLeaf. Since it deletes a heap, there's nothing for it to return. So, how do you delete a heap? Well, if the heap is a leaf (it has no left or right subtree), delete that, else delete the subtrees by calling DeleteHeap recursively. Code that and you're done.

Edit: You left a comment:

deleteLeaf is supposed to delete the last element of the tree at the last level. The value returned should be the data contained in the deleted leaf.

Well, that's news. We aren't mind-readers. Your question didn't say this, and the function name and signature are wrong for this, too.

Let's start with the name -- DeleteRightmostLeaf. And the return type ... data_t*. Even the argument type is wrong ... it should be heap_t**, because we have to store a NULL into the pointer.

So, DeleteRightmostLeaf takes a pointer to a pointer to a heap. If that heap is a leaf node, store NULL in the pointer to it, extract its data pointer, free the node (in that order ... otherwise you're accessing deleted memory, which isn't allowed), and return the data pointer.

If the heap isn't a leaf node, then call DeleteRightmostLeaf recursively on the pointer to its rightmost subtree -- the right subtree if that's not NULL, else the left subtree. Voila, you're done.

Note that, in both cases, it's very easy to come up with the answer if one just thinks clearly about what they need to do.

As a bonus, here's an iterative solution. I haven't tested or even compiled this.

data_t* DeleteRightmostLeaf(node_t** pheap)
{
    node_t* pnode = *pheap;

    if (!pnode)
        return NULL; // empty heap

    while (pnode->left || pnode->right)
    {
        pheap = pnode->right ? &pnode->right : &pnode->left;
        pnode = *pheap;
    }

    *pheap = NULL;
    data_t* pdata = pnode->data;
    free(pnode);
    return pdata;
}

Answer 2

To delete a node in a tree you need to

1. free the memory and do the cleanup for the node
2. fix the pointer you used to reach the node, making it NULL

the part 2 can be solved in two ways:

A) the parent does the fixing B) the deletion routine receives the address of the address of the node (extra level of indirection).

For solution A the code is simply

void deleteNodeA(Node *p) {
    if (p) {
        // Here we don't really need part 2 because
        // we're going to destroy the whole node containing
        // the pointers anyway.
        deleteNodeA(p->left);  // add p->left = NULL if you like
        deleteNodeA(p->right); // add p->right = NULL if you like
        free(p->data);
        free(p);
    }
}

but the caller needs to fix the pointer used to reach the node. For example like

Node *p = root, *parent = NULL;
while (p && (p->left || p->right)) {
    // Not a leaf... go left if possible o right otherwise
    parent = p;
    p = p->left ? p->left : p->right;
}

// 2: Fix the pointer in parent
if (parent) {
    if (p == parent->left) {
        parent->left = NULL;
    } else {
        parent->right = NULL;
    }
} else {
    // No parent... this was the root of the tree
    root = NULL;
}

deleteNodeA(p);

The solution B looks like:

void deleteNodeB(Node **p) { // Note the double pointer
    if (*p) {
        deleteNode(&((*p)->left));  // Note the &
        deleteNode(&((*p)->right)); // Note the &
        free((*p)->data);
        free(*p);
        *p = NULL; // (2): fixing the pointer
    }
}

and for example code deleting a leaf of the tree is

Node **p = &root;
while ((*p) && ((*p)->left || (*p)->right)) {
    // Not a leaf... go left if possible o right otherwise
    p = ((*p)->left) ? &((*p)->left) : &((*p)->right));
}
deleteNodeB(p);

Answer 3

Try this modification of your method:

node_t* deleteLeaf(node_t* heap)
{
  if (!heap)
    return 0;

  if (heap->left!=0)
    deleteLeaf(heap->left);
  if (heap->right!=0)
    deleteLeaf(heap->right);

  if (heap->data)
    free(heap->data); // free data
  free(heap); // free leaf
  heap = 0;
  return heap;
}

One question: which value should be returned by this function? (now it always returns 0). It is hard to understand what you are trying to do (we haven't description of the function, examples of expected results and so on). So, I suspect, that code above is not solution. But it might be first step in understanding of the problem.