1

There is no error when I compile the code, but the program crashes at runtime after two inputs. Maybe there is some logical error that I can not make out. I'm trying to insert nodes at the tail of linked lists while only maintaining head position.

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

struct Node{

    int data;
    struct Node* next;
};

struct Node *head;

//print the element of the lists
void print(){
    printf("\nThe list from head to tail is as follows \n");
    struct Node* temp = head;
    while(temp!=NULL){
        printf("\n %d ",(*temp).data);
        temp = (*temp).next;
    }
}

//insert a node at the tail of the linked list
void insert_at_tail(int data){
    struct Node* temp = head;
    struct Node* new_node = (struct Node*)malloc(sizeof(struct Node));
    new_node->data=data;
    new_node->next=NULL;

    if(temp==NULL){
        head=new_node;
    }
    else{
        while(temp!=NULL){temp=temp->next;}
        (*temp).next=new_node;
    }
}
int main(){

    head = NULL;
    int i,data;
    for(i=0;i<5;i++){
        scanf("%d",&data);
        insert_at_tail(data);
    }
    print();

    return 0;
}
3
  • for ease of readability and understanding:: 1) separate code blocks (for, if, else, while, do...while, switch, case, default) via a single blank line 2) follow the axiom: only one statement per line and (at most) one variable declaration per statement. Commented Jun 30, 2017 at 13:31
  • when calling scanf(), always check the returned value (not the parameter values) to assure the operation was successful. Commented Jun 30, 2017 at 13:32
  • when calling any of the heap allocation functions (malloc, calloc, realloc), 1) always check (!=NULL) the returned value to assure the operation was successful. 2) the returned type is void* so can be assigned to any pointer. Casting just clutters the code, making it much more difficult to understand, debug, etc. Commented Jun 30, 2017 at 13:34

1 Answer 1

5

Maybe there is some logical error?

Yes!

Here:

while(temp!=NULL) { temp=temp->next; }
(*temp).next=new_node;

you will loop until temp is actually NULL and then request its next member, so you are asking for next of NULL, thus you are asking for trouble (the program crashes)!

Try doing this instead:

while(temp->next != NULL) { temp=temp->next; }

where you are looping until temp points to the last node of your list. With that change your code should work fine.


PS: Do I cast the result of malloc? No!

Sign up to request clarification or add additional context in comments.

7 Comments

while(temp!=NULL){temp=temp->next;} temp=new_node; this worked.
@EshanPandey I think you should follow my solution, since I tested it works nicely. Your approach doesn't connect the nodes.
It is not like I did not like your solution, just that I came up with my own. Obviously, your answer helped. You pointed out what I was doing wrong, which helped me. @gsamaras Thanks. BTW still struggling with print() function.
Yes because you do not link the nodes. The next member of every node of yours points to NULL with your solution, while with mine's, every node points to its next node. That's why you can't print your list @EshanPandey.
Yes, I think you are right. My approach does not connect the nodes and therefore the entire list is not printed.. only 1st element. But I don't understand this completely. Can you help (explain)
|

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.