Various issues with binary tree minheap implementation in C

Question

I'm trying to implement a binary search tree in C by using minheap. I've got the basic code down but I cannot get it to work properly. My main problem, according to the build messages, seem to be that I compare pointers and integers. As this code is based off of various explanations that I found in my textbooks and online, I'm not sure how to avoid that.

Here is the complete code (final and working - see edit history for original):

#include <stdio.h>
#include <stdlib.h>

typedef struct TreeNode {
    int key;
    struct TreeNode* lChild;
    struct TreeNode* rChild;
} node;

node *createTree(int key) {

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

    root->key = key;
    root->lChild = NULL;
    root->rChild = NULL;

    return(root);

}

void insert(node *root, int key) {

    if (root->key == -1)
        root->key = key;

    else if (key < root->key) {

        if (root->lChild != NULL)
            insert(root->lChild, key);

        else {
            root->lChild = malloc(sizeof(node));
            root->lChild->key = key;
            root->lChild->lChild = NULL;
            root->lChild->rChild = NULL;
        }
    }

    else if (key > root->key) {

        if (root->rChild != NULL)
            insert(root->rChild, key);

        else {
            root->rChild = malloc(sizeof(node));
            root->rChild->key = key;
            root->rChild->lChild = NULL;
            root->rChild->rChild = NULL;
        }
    }

}

void deleteTree(node *root) {

    if (root != NULL) {

        if (root->lChild != NULL)
            deleteTree(root->lChild);

        if (root->rChild != NULL)
            deleteTree(root->rChild);

        free(root);
    }

}

void printNode(node *root) {

    if (root->lChild != NULL) {

        printf("%d -- %d\n", root->key, root->lChild->key);

        if (root->rChild != NULL)
            printf("%d -- %d\n", root->key, root->rChild->key);

        printNode(root->lChild);
    }

    if (root->rChild != NULL) {

        if (root->lChild == NULL)
            printf("%d -- %d\n", root->key, root->rChild->key);

        printNode(root->rChild);
    }
}

void printTree(node *root) {

    printf("graph g {\n");

    printNode(root);

    printf("}\n");
}

void test() {

    // 1.
    node *root1 = createTree(-1);

    insert(root1, 4);
    insert(root1, 2);
    insert(root1, 3);
    insert(root1, 8);
    insert(root1, 6);
    insert(root1, 7);
    insert(root1, 9);
    insert(root1, 12);
    insert(root1, 1);
    printTree(root1);

    // 2.
    node *root2 = createTree(-1);

    insert(root2, 3);
    insert(root2, 8);
    insert(root2, 10);
    insert(root2, 1);
    insert(root2, 7);
    printTree(root2);

    // 3.
    deleteTree(root1);

    // 4.
    deleteTree(root2);

}

int main() {
    test();
}

Here is the output (final and working - see edit history for original):

graph g {
4 -- 2
4 -- 8
2 -- 1
2 -- 3
8 -- 6
8 -- 9
6 -- 7
9 -- 12
}
graph g {
3 -- 1
3 -- 8
8 -- 7
8 -- 10
}

Process returned 1 (0x1)   execution time : 0.712 s
Press any key to continue.

It looks like I need to exclude the connection from "nothing" to the root of the tree still.

printTree I added for debugging purposes but there seem to be problems with it as well.

Answer 1

First, when you have compiler warnings messages like this, do not try to execute. It means you shall obviously memory leaks somewhere.

And your problem is effectively that you are making a comparaison between an integer and a pointer here :

(root->key == NULL)

According to your structure, root->key is an integer, looks like your node data. But NULL is using to reference as a non-assigned pointer.

You don't have to use your key as a "check" to know if your node is free or not. Every node in a tree has already an assigned value. You should better travel your tree until you reach a NULL node, and then allocate this node from it parent. And that's exactly what you are doing in your heapify function.

In fact, the only thing you should do is to remove your insert function, calling directly heapify instead.

You have another problem here:

printf("key: %d, lChild key: %d, rChild key: %d\n", root->key, root->lChild->key, root->rChild->key);

if (root->lChild != NULL)
[...]

You are printing data in node childs before checking if there is a child at all. That's a big potential source of core dumps.

I recommend to use a recursive algorithm for your printNode as for your other functions :

void printNode(node *root) {
    if (root->lChild != NULL) {
        printf("Left child: ");
        printf("%d -- %d\n", root->key, root->lChild->key);
        printNode(root->rChild);
    }
    if (root->rChild != NULL) {
        printf("Right child: ");
        printf("%d -- %d\n", root->key, root->rChild->key);
        printNode(root->rChild);
    }
}

And for your root key problem, I suggest to make a distinct function to create your tree, taking as parameter your first key :

node *createTree(int rootKey) {
    node *root;

    root = malloc(sizeof(node));
    if (root == NULL) return NULL; // Don't forget to check your malloc's return !
    root->key = rootKey;
    root->lChild = NULL;
    root->rChild = NULL;

    return (root);
}

Answer 2

There is a problem in the printTree function:

printf("key: %d, lChild key: %d, rChild key: %d\n", root->key, root->lChild->key, root->rChild->key);

You don't check if root->lChild or root->rChild are NULL. This yields a segmentation fault. If you are using Linux, I would recommend to use valgrind. It will help you debugging C programs, and save you a lot of time.