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;
}
gsamaras
  • 71,951
  • 46
  • 188
  • 305
Eshan Pandey
  • 55
  • 8
  • 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.* – user3629249 Jun 30 '17 at 13:31
  • when calling `scanf()`, always check the returned value (not the parameter values) to assure the operation was successful. – user3629249 Jun 30 '17 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. – user3629249 Jun 30 '17 at 13:34

1 Answers1

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!

gsamaras
  • 71,951
  • 46
  • 188
  • 305
  • while(temp!=NULL){temp=temp->next;} temp=new_node; this worked. – Eshan Pandey Jun 28 '17 at 20:02
  • @EshanPandey I think you should follow my solution, since I tested it works nicely. Your approach doesn't connect the nodes. – gsamaras Jun 28 '17 at 20:18
  • 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. – Eshan Pandey Jun 28 '17 at 20:25
  • 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. – gsamaras Jun 28 '17 at 20:28
  • 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) – Eshan Pandey Jun 28 '17 at 20:32
  • Thanks.. @gsamaras got it now. – Eshan Pandey Jun 28 '17 at 20:33
  • @EshanPandey get a piece of paper and **draw** what your code does, that will help! With your approach the `next` member of every node points to `NULL`, thus when you try to print the list (thus iterate over it), you ask for the `next` member, which is `NULL`, thus your first node thinks that it is the last as well (because it's `next` is `NULL` incorrectly!). – gsamaras Jun 28 '17 at 20:34