Binary Tree Insertion Error

Question

I looked at several BST Insert articles but none of them were structured the same as mine or encountered the same problem.

My problem is my Binary Tree isn't being built correctly. It's really strange because I'm copy+pasting most of the code from a previous project where it works fine, the only difference is the data the nodes contain, and the condition for looping through the tree uses strcmp rather than integer comparison.

This is my insert function:

//insert parameter node data into a Binary Tree
TreeNodePtr insertNode(BinaryTree bst, Record d)
{
    //if root is null insert into root
    if(bst.root == NULL)
    {
        bst.root = (TreeNodePtr) malloc(sizeof(TreeNode));
        bst.root->data = d;
        bst.root->left = NULL;
        bst.root->right = NULL;
        return bst.root;
    }

    //we need to traverse the tree so declare a pointer "curr" to do so
    TreeNodePtr curr = (TreeNodePtr) malloc(sizeof(TreeNode));
    curr = bst.root;

    //loop until we find an appropriate empty space or a match (no duplicates)
    while (strcmp(d.lastName, curr->data.lastName) != 0)
    {
        if (strcmp(d.lastName, curr->data.lastName) < 0)
        { // if left
            if(curr->left==NULL) 
            {
                curr->left = (TreeNodePtr) malloc(sizeof(TreeNode));
                curr->left->data = d;
                curr->left->left = NULL;
                curr->left->right = NULL;
                return bst.root;
            }
            curr=curr->left;
        }
        else if (strcmp(d.lastName, curr->data.lastName) > 0)
        { // try right
            if(curr->right==NULL)
            {
                curr->right = (TreeNodePtr) malloc(sizeof(TreeNode));
                curr->right->data = d;
                curr->right->left = NULL;
                curr->right->right = NULL;
                return bst.root;
            }
            curr=curr->right;
        }
    }

    return bst.root;
}

Here is the code in the main function which uses the insert function to build the tree (note that records is a correctly populated array, each index containing one node's data):

//declare BST and build it
BinaryTree phoneTree;
phoneTree.root = NULL;

for (int i=0; i < sizeof(records) / sizeof(Record); i++)
{
    Record tmpRecord;
    tmpRecord.firstName = records[i].firstName;
    tmpRecord.lastName = records[i].lastName;
    tmpRecord.phoneNum = records[i].phoneNum;
    phoneTree.root = insertNode(phoneTree, tmpRecord);
}

And for reference, here are the tree structs:

//phone data record struct
typedef struct
{
    char *firstName;
    char *lastName;
    char *phoneNum;
}Record;

//define the tree node which contains the data
typedef struct treeNode
{
    Record data;
    struct treeNode *left,*right;
}TreeNode,*TreeNodePtr;

//define binary tree struct
typedef struct
{
    TreeNodePtr root;
}BinaryTree;

I've been staring at the program that works and comparing it to this program for about 5 hours now and I can't figure out what's going wrong. I know the tree isn't populated correctly because if i try to print phoneTree.root->right.data or phoneTree.root->left.data attributes, the program crashes. In the program I'm borrowing the code from, these attributes are printed without error. The root is still inserted correctly and it's attributes can be printed.

Any insight as to what I'm doing incorrectly is greatly appreciated.

Answer 1

Thanks for all the great tips, they've all contributed to my understanding of memory management in C.

Strangely, I found the problem is actually rooted in my array for loop. I found the method of using sizeof(array) / sizeof(arraydatatype) from multiple sources on the internet and this site so I attempted it, but it doesn't work the way I tried. In:

for (int i=0; i < sizeof(records) / sizeof(Record); i++)
{
    Record tmpRecord;
    tmpRecord.firstName = records[i].firstName;
    tmpRecord.lastName = records[i].lastName;
    tmpRecord.phoneNum = records[i].phoneNum;
    phoneTree.root = insertNode(phoneTree, tmpRecord);
}

I replaced "i < sizeof(records) / sizeof(Record)" with just"i < 3" (array should only have 3 elements at this point), and everything worked as it should. It's a really dumb source of the problem, but funny that despite all the answers provided none mentioned it :p

Since we're already here, can anyone explain why that was going wrong / how to properly loop through an array in such a manner?

Answer 2

There is one definite mistake, which could be causing you problems. You need to pass "bst" by reference, so that the function can modify "bst.root". Try rewriting the function as:

 TreeNodePtr insertNode(BinaryTree* bst, Record d)

and use "bst->" in place of "bst."

You said that it worked with integers. Now that may be a clue to another mistake. Your record contains only pointers to strings. Do these pointers remain valid throughout the lifetime of the tree? Maybe you need to make copies of the strings within the record.

Couple of other minor things:

//we need to traverse the tree so declare a pointer "curr" to do so
TreeNodePtr curr = (TreeNodePtr) malloc(sizeof(TreeNode));
curr = bst.root;

malloc is redundant here, the result is immediately overwritten.

And:

    }
    else if (strcmp(d.lastName, curr->data.lastName) > 0)
    { // try right

you can replace this with "} else {" as you already did this strcmp operation.