Binary Tree implementation: issues with insert_tree()

Viewed 58

I'm starting to learn a litte bit of C after I did C++, so I'm with a bintree implementation.

Code:

struct Node {
    int value = -1;
    struct Node* left = NULL;
    struct Node* right = NULL;
};

void insert_tree(int value, Node* root) {
    if (root == NULL) {
        root = (struct Node*) malloc(sizeof(Node));
        root->value = value;
        printf("%d\n", root->value);
        root->left = NULL;
        root->right = NULL;
    }
    else {
        if (root->value >= value) {
            insert_tree(value, root->left);
        }
        else {
            insert_tree(value, root->right);
        }
    }
}

void printTree(Node* root) {
    if (root != NULL) {
        printf("%i\n", root->value);
        printTree(root->left);
        printTree(root->right);
    }
}

int main() {
    Node* root = NULL;
    insert_tree(30, root);
    printTree(root);
}

Here I can see how insert_tree is doing the malloc, and allocating correctly the memory, so it outputs 30 inside insert_tree, but when I call printTree() there are no nodes in the tree. I don't know why, since I'm passing the left pointer of the node, and it should be safe from the context of insert_tree.

What is the problem with insert_tree() here?

2 Answers

For starters this structure declaration

struct Node {
    int value = -1;
    struct Node* left = NULL;
    struct Node* right = NULL;
};

is invalid in C. You may not specify initializers.

So the declaration shall look like

struct Node {
    int value;
    struct Node* left;
    struct Node* right;
};

The function insert_tree gets the pointer to the root node by value. That is the function deals with a copy of the original pointer, So changing a copy of the original pointer within the function does not influence on the value of the original pointer.

You need to pass the pointer by reference. In C passing by reference means passing an object indirectly through pointer. Take into account that memory allocation can fail. It is desirable that the function would report whether insertion of a new node was successful.

Also it will be more competently to make the first parameter the pointer to the pointer to the root node and the second parameter the value that should be inserted.

In any case this function declaration

void insert_tree(int value, Node* root) {
                            ^^^^^^^^^^

is an invalid C function declaration because opposite to C++ in C you have to specify the keyword struct as

void insert_tree(int value, struct Node* root) {
                            ^^^^^^^^^^^^^^^^^ 

The recursive function insert_tree can be defined the following way

int insert_tree( struct Node **root, int value ) 
{
    if ( *root == NULL ) 
    {
        *root = (struct Node*) malloc( sizeof( struct Node ) );
        if ( *root != NULL )
        {
            ( *root )->value = value;
            ( *root )->left  = NULL;
            ( *root )->right = NULL;
        }

        return *root != NULL;
    }
    else 
    {
        if ( value < ( *root )->value ) 
        {
            return insert_tree( &( *root )->left, value );
        }
        else 
        {
            return insert_tree( &( *root )->right, value );
        }
    }
}

And the function can be called like

int main( void ) 
{
    struct Node *root = NULL;
    insert_tree( &root, 30 );
    printTree( root );
}

Also as the function printTree does not change the tree then its parameter should have the qualifier const.

void printTree( const struct Node* root ) {
    if (root != NULL) {
        printf("%i\n", root->value);
        printTree(root->left);
        printTree(root->right);
    }
}

Here is a demonstrative C program.

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

struct Node 
{
    int value;
    struct Node *left;
    struct Node *right;
};

int insert_tree( struct Node **root, int value ) 
{
    if ( *root == NULL ) 
    {
        *root = (struct Node*) malloc( sizeof( struct Node ) );
        if ( *root != NULL )
        {
            ( *root )->value = value;
            ( *root )->left  = NULL;
            ( *root )->right = NULL;
        }

        return *root != NULL;
    }
    else 
    {
        if ( value < ( *root )->value ) 
        {
            return insert_tree( &( *root )->left, value );
        }
        else 
        {
            return insert_tree( &( *root )->right, value );
        }
    }
}

void printTree( const struct Node* root ) {
    if (root != NULL) {
        printf("%i\n", root->value);
        printTree(root->left);
        printTree(root->right);
    }
}

int main(void) 
{
    struct Node *root = NULL;

    const int N = 10;

    srand( ( unsigned int )time( NULL ) );

    for ( int i = 0; i < N; i++ )
    {
        insert_tree( &root, rand() % N );
    }

    printTree( root );

    return 0;
}

Its output might look like

1
0
9
4
3
7
5
6
5
8

You should pass root by reference , change this:

void insert_tree(int value, Node* root) {
    if (root == NULL) {
        root = (struct Node*) malloc(sizeof(Node));
        root->value = value;
        printf("%d\n", root->value);
        root->left = NULL;
        root->right = NULL;
    }
    else {
        if (root->value >= value) {
            insert_tree(value, root->left);
        }
        else {
            insert_tree(value, root->right);
        }
    }
}

to this:

void insert_tree(int value, Node** rootp) {
    struct Node * root = *rootp;
    if (root == NULL) {
        root = (struct Node*) malloc(sizeof(Node));
        root->value = value;
        printf("%d\n", root->value);
        root->left = NULL;
        root->right = NULL;
    }
    else {
        if (root->value >= value) {
            insert_tree(value, &root->left);
        }
        else {
            insert_tree(value, &root->right);
        }
    }
}

And then call insert like this:

insert_tree(value , &root);

This way you ensure that any changes made to the variable within the function are kept. This happens because when you instead pass by value, the function creates a copy of the variable you added as a parameter , and thus any changes to it won't affect the original value.

edit: Just to clarify,pointers are still variables,and as variables they hold a value. In order to change a variable's value within a function,you need to get its address , a pointer's address just like any other variable's address is accessed through & . If Root( the variable you declared in main ) wasn't a pointer,then you could as well be fine using a normal struct Node * root as a parameter,but since it isn't , you should stick to struct Node ** root.

Related