what is wrong with below tree element insert

struct node* insert(struct node* node, int data)
{
    if(node == NULL)
    {
        // if tree is empty
        return NewNode( data ); 
    }
    else
    {
        if( node->data > data )
        {
            // data is less than node, add to left subtree
           return  node->left = insert(node->left, data);
        }
        else if( node->data <= data)
        {
            // data is more than node, add to right subtree
            return node->right = insert(node->right, data);
        }
            // else return node
            return node;
    }
}

called with

node *p = new nod开发者_如何学运维e();
p->data = 2;
//printf("%d",lookup(p,2));

 insert( p, 3);
 insert( p, 4);
 insert( p, 5);
 PrintPreOrder(p);

---

returns : 2,5

---

void PrintPreOrder(node *node)
{
    if(node==NULL)
    {
        return;
    }
    else
    {
        printf("%d ", node->data);

        PrintPreOrder(node->left);
        PrintPreOrder(node->right);
    }
} 

---

The problem is that insert function in your case should always return current node. Something like this:

struct node* insert(struct node* node, int data)
{
    if(node == NULL)
    {
        // if tree is empty
        return NewNode( data ); 
    }
    else
    {
        if( node->data > data )
        {
            // data is less than node, add to left subtree
           node->left = insert(node->left, data);
            // <<<<<<<<<<< NO RETURN HERE
        }
        else if( node->data <= data)
        {
            // data is more than node, add to right subtree
            node->right = insert(node->right, data);
            // <<<<<<<<<<< NO RETURN HERE
        }
        // else return node
        //ALWAYS RETURN NODE
        return node;
    }
}

Operator = returns the value that was set on the right side of it - in your case that's the bottom-most created child node which eventually overwrites one of the top nodes.

---

You're assigning values in too many places.

return node->left = insert(node->left, data);
// ...
return node->right = insert(node->right, data);

These lines are returning the value, and assigning - at each level of the recursion stack.

You shouldn't assign repeatedly while recursing. Otherwise, you'll tear down the tree you're traversing by replacing each node in the traversal with the final result node.

With code structured like this (with the left/right recursion + return), the only place you should do the assignment is at:

if(node == NULL)
{
    // assign here...
}

If you do this, though, you'll have to take a pointer-to-pointer.