Binary Search Tree node insertion not working properly

Question

I have a homework assignment to implement a binary search tree (create, delete, search). I used the example provided by the teacher but I can't make it work.

Here's my code so far:

void insert_node(int k){
    struct node *nodnou,*flow,*parent;

    nodnou = (struct node*)malloc(sizeof(node));
    nodnou->st = NULL;
    nodnou->dr = NULL;
    nodnou->nr = k;

    if (root==NULL)
    {
        root = (struct node*)malloc(sizeof(node));

        root = nodnou;
    }
    else
    {
        flow = (struct node*)malloc(sizeof(node));
        parent = (struct node*)malloc(sizeof(node));
        flow = root;
        parent = root;
        while (((flow->st!=NULL) || (flow->dr!=NULL)) && flow!=NULL)
        {
            if (k<flow->nr)
            {
                parent = flow;
                flow = flow->st;
            }
            else
            {
                parent = flow;
                flow = flow->dr;
            }
        }

        if (k<flow->nr)
        {
            parent->st = nodnou;
        }
        else
        {
            parent->dr = nodnou;
        }
    }
}

The way of thinking: This function gets the value of the node we want to insert as the k parameter. The function will only insert the root of the tree (root is global variable).

I think my biggest problem is the while loop that sweeps through the tree to find the spot for the new node.

If I use while (flow!=NULL) it won't work because the flow pointer gets an assignment to something that does not exist. Please help me understand where I am wrong (homework).

Answer 1

Your code has several important flaws, not the least of which is a misunderstanding of how dynamic memory allocation works in C. Never follow a pattern like this:

Type *pointer = malloc(sizeof(Type));
pointer = <<something else>>

It literally leaks memory and gains you nothing in two short lines. This isn't an object-reference based language like Java or C#. Pointers are variables that hold memory addresses. Just like an int can hold an integer, a pointer holds an address. And just like the following example:

int n = 6;
n = 5;      //Hmm. Where did the 6 go? Oh yeah, We overwrote it with 5. 

You will lose your allocation link doing the same thing with pointers:

struct node *root = malloc(sizeof(*root));
root = nodnou; // memory allocated above is gone. forever leaked.

---

Pointers are variables. Just like any other variable, they hold values. In the case of a pointer, however, its value is an address. You can have pointers to almost anything in C, including pointers to pointers; variables that hold the address of pointer variables. And I bring them up because they proffer a particularly elegant solution to your insertion requirements.

The following is a general implementation for a binary tree insertion that supports no duplicates in the tree (the code gets even shorter if you allow duplicates). Furthermore, it does this using exactly zero local variables beyond the provided function parameters, and I challenge you to dissect this and determine how it works. It even works on an initially NULL tree root pointer, eliminating the need to special casing if (root) {} else {} logic:

void insert_node(struct node **pp, int k) 
{
    while (*pp)
    {
        if (k < (*pp)->nr)        // move down left side?
            pp = &(*pp)->st;

        else if ((*pp)->nr < k)   // move down right side?
            pp = &(*pp)->dr;

        else return;              // found the key, no dupes. leave
    }

    // pp contains the address of the pointer we need to set.
    *pp = malloc(sizeof(**pp)); 
    (*pp)->st = (*pp)->dr = NULL;
    (*pp)->nr = k;
}

If your tree should support duplicates you need to be consistent about which side they are inserted on, but it shortens the above algorithm considerably:

void insert_node(struct node **pp, int k) 
{
    while (*pp)
        pp = (k < (*pp)->nr) ? &(*pp)->st : &(*pp)->dr;

    // pp contains the address of the pointer we need to set.
    *pp = malloc(sizeof(**pp)); 
    (*pp)->st = (*pp)->dr = NULL;
    (*pp)->nr = k;
}

In either case, invoked on the caller side like this:

struct node *root = NULL;

insert(&root, 5);
insert(&root, 10);
insert(&root, 7); 
...etc...

Answer 2

I think you should use while(flow != NULL) and insert your element as flow after that. The way it is right now it will stop in cases when it shouldn't and do weird things whenever it stops. Try working through some examples with pen and paper.

Answer 3

You almost got it. Keep Up!

First you need to understand a bit better memory allocation. In reality, you only need the very first malloc() call in your function. That is the memory you allocate for the node you are appending to the tree during each insert_node() call. All remainingr mallocs you are performing are unnecesary. It seems that you intuitively feel you need to allocate memory for the other pointers you are using, but all of them are temporary and don't require any allocation, just assignment to a valid node before attempting to de-reference them. In fact, those unnecesary allocations will create what is known as a memory leak (memory you request and fail to release) in code like this:

root = (struct node*)malloc(sizeof(node));
root = nodnou;

The second assignmet (root = nodnou) overwrites the result of the previous malloc() call and since you didn't save the overwritten pointer value in any other place, you will no longer be able to release that memory, it will be marked as used for the lifetime of your application!

Next, you can simplify the code that is walking the tree looking for the insertion point. You seem to worry that flow becomes NULL, but it doesn't matter. The important node is parent. After the while loop ends, it will be pointing to the actual node where the inserted node needs to be linked. Here is a modified version of your code.

void insert_node(int k) {
   struct node *nodnou, *flow, *parent;
   // this is the only memory allocation that should be done
   nodnou = (struct node*)malloc(sizeof(node));
   nodnou->st = NULL;
   nodnou->dr = NULL;
   nodnou->nr = k;

   parent = NULL;

   if( root == NULL ) {
      root = nodnou;
   } else {

      flow = root;
      // We will walk the tree in order until we reach the bottom of the 
      // tree (flow becomes null). What we are trying to do is to find out
      // the node that will become the parent of the new node we are inserting
      // It doesn't matter if flow becomes NULL, the important value after the
      // while loop ends is parent 
      while( flow != NULL ) {
         // update the current potential parent node
         parent = flow;
         if( k < flow->nr ) {
            // the number we are inserting is lower than the flow node, walk to the left
            flow = flow->st;
         } else {
            // the number we are inserting is greater or equal than the flow node, 
            // walk to the right
            flow = flow->dr;
         }
      }

      // We have reached the bottom, now compare number again with the node that
      // will become parent, to find out if we need to link this node to the left
      // or to the right of the parent node

      if( k < parent->nr ) {
         parent->st = nodnou;
      } else {
         parent->dr = nodnou;
      }
   }

}

That's it. Try to code the rest of the tree operations and don't hesitate to ask if you become confused. =)