0

This is homework for my first class in c. It focuses on dynamic allocation in c, in the form of a bst.

I have to have a dynamically allocated BST, recursively implemented. I know that my traversal works correctly, and am having trouble inserting nodes. I only ever have the root node, and every other node seems to be set to NULL. I think that I can't print the rest of the nodes when traversing, because I am trying to access the data member of a NULL struct. My code so far is as follows:

void insert_node(TreeNode** root, int toInsert){
    if(*root==NULL){
        TreeNode *newnode = (TreeNode *)malloc(sizeof(TreeNode));
        newnode->data = toInsert;
        newnode->left = NULL;
        newnode->right = NULL;
    }
    else if(toInsert > (*root)->data){ //if toInsert greater than current
        struct TreeNode **temp = (TreeNode **)malloc(sizeof(struct TreeNode*));
        *temp = (*root)->right;
        insert_node(temp, toInsert);
    }
    else{ //if toInsert is less than or equal to current
        struct TreeNode **temp = (TreeNode **)malloc(sizeof(struct TreeNode*));
        *temp = (*root)->left;
        insert_node(temp, toInsert);
    }
}

void build_tree(TreeNode** root, const int elements[], const int count){
    if(count > 0){
        TreeNode *newroot = (TreeNode *)malloc(sizeof(TreeNode));
        newroot->data = elements[0];
        newroot->left = NULL;
        newroot->right = NULL;
        *root = newroot;
        for(int i = 1; i < count; i++){
            insert_node(root, elements[i]);
        }
}

I'm sure it's only one of many problems, but I get segmentation faults on any line that uses "(*root)->data", and I'm not sure why.

As a side note, despite getting segmentation faults for the "(*root)->data" lines, I'm still able to printf "(*root)->data". How is it possible to print the value, but still get a segmentation fault?

Improve this question
1
  • Your don't attach your newnode to the tree in insert_node. Commented Mar 19, 2014 at 1:06

1 Answer 1

Reset to default
0

It's messy. Some things that might help

1) Don't need to use TreeNode*, pointer to pointer, as argument. Use jsut the TreeNode. (something went wrong here, as it's some feature from the text editor, consider and additional * after each TreeNode in this line)

2) Not a strict rule, but as best practice avoid using the first node of a linked list to store actual values. Use just as the header of your list. Reason is, if you need to delete this node, you don't lose the list. Just a tip

3) In your first function, if *root==NULL, I'd rather make the function fail than adding it to a temporary list (that's being lost in the current code, see that it adds the value to a list that is not being passed outside the function.

4) Well, you are actually making it go to the right if the new value is greater than the node, to the left if it's smaller than the node, but it never stops. See this example: Suppose you have the list 1->3->4. Now you want to insert 2. What the algorithm will do? keep trying to insert in the 1 node and 3 node, switching between them, but never actually inserting anything. Solution: as you will build this list bottom up, your list will always be sorted (inf you insert nodes correctly). So you just need to check if the next node is higher, and if it is, insert right where you are.

5) If you're passing a TreeNode *root as argument (on the 2nd function), you shouldn't have to recreate a new list and make root=newlist. Just use the root. All of this would result in (didn't test, might be some errors):

void insert_node(TreeNode* root, int toInsert){
if(root==NULL){
    printf("Error");
    return;
}
TreeNode* temp = root; //I just don't like to mess with the original list, rather do this
if(temp->right!=NULL && toInsert > temp->right->data){ //if toInsert greater than next
    insert_node(temp->right, toInsert);
}
else{ //if toInsert is less or equal than next node
    TreeNode* temp2 = temp->right; //grabbing the list after this node
    temp->right=(TreeNode*)malloc(sizeof(TreeNode)); //making room for the new node
    temp->right->right=temp2; //putting the pointer to the right position
    temp->right->left=temp; //setting the left of the next node to the current
    temp->right->data=toInsert;
}
}

void build_tree(TreeNode* root, const int elements[], const int count){
if(count > 0){
    for(int i = 0; i < count; i++){
        insert_node(root, elements[i]);
    }
}
}
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

And a couple of comments. Never used left and right on C, not sure if it will work. I'd always rather using linked lists the old-fashioned way (data and pointer to the next). You actually don't need recursion, but if it's a requirement for the assigment, okay.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.