Why the following implementation overrides the elements/ or prints only one element

Question

I am trying to implement linked list in c. In the insertion of the element, if the head is not NULL, I am trying to add a node in the beginning of the linked list Here is my code

#include<stdio.h>
#include<stdlib.h>
struct Node{
    int data;
    struct Node* next;
};

struct Node* head ;

 void insert(int data){

     struct Node* temp = (struct Node*) malloc(sizeof(struct Node));

    temp->data = data;
    if(head!=NULL){
        temp = head;
        head = temp;
     }

    temp -> next = NULL;
    head = temp;

 }


void print(){

    struct Node* temp = head;
    while(temp!=NULL){
        printf("%d \n",temp->data);
        temp = temp->next;
    }
}


int main(){
  head = NULL;
  insert(2);
  insert(3);
  insert(5);
  print();

    return 0;
}

But on print function, I am getting only 2 as output. What might be the reason?

Your insert function doesn't work properly when `head!=NULL`. You're throwing away the allocated `temp` node. Recommended reading: [How to debug small programs](http://ericlippert.com/2014/03/05/how-to-debug-small-programs/). In this case, you would probably have solved the problem yourself if you added some prints in `insert()`, or just manually thought through the program while drawing the changes to the list. — Snild Dolkow, Oct 26 '15 at 06:46

Vlad from Moscow · Accepted Answer · 2015-10-26T07:01:13.653

1

The function should be defined the following way

void insert( int data )
{
    struct Node *temp = ( struct Node * )malloc( sizeof( struct Node ) );

    if ( temp != NULL )
    {
        temp->data = data;
        temp->next = head;

        head = temp;
    }
}

Or the following way

_Bool insert( int data )
{
    _Bool success;

    struct Node *temp = ( struct Node * )malloc( sizeof( struct Node ) );

    if ( ( success = temp != NULL ) )
    {
        temp->data = data;
        temp->next = head;

        head = temp;
    }

    return success;
}

As for your code then you always assign head itself when it is not the first node

if(head!=NULL){
    temp = head;
    head = temp;
 }

And as result the program has memory leaks and the list contains always the first inserted element.

Of course you need to write also a function that will free all allocated memory when the list is not needed any more.

edited Oct 26 '15 at 07:01

answered Oct 26 '15 at 06:39

Vlad from Moscow

301,070
26
186
335

head!=NULL is checking if the list is empty. But you are checking temp!=NULL. I don't understand – gates Oct 26 '15 at 06:47
@gates `temp != NULL` checks whether or not the memory allocation succeeded – M.M Oct 26 '15 at 06:48
The result of `malloc` [should not be cast](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) – M.M Oct 26 '15 at 06:48
@gates I check whether the memory was allocated successfully. – Vlad from Moscow Oct 26 '15 at 06:49
Assuming memory allocation is not a problem. How do I check whether the list is empty or not – gates Oct 26 '15 at 06:50
`temp != NULL` checks that the malloc succeeded. Actually, Vlad's code silently ignores the failure case, just skipping the insert if the malloc fails. That's probably not a good idea. For smaller programs, we usually just print an error and exit if a malloc fails. – Snild Dolkow Oct 26 '15 at 06:50
@gates You need not check this. The code does not depend on whether head is NULL or not. – Vlad from Moscow Oct 26 '15 at 06:51
`head != NULL` is the way to check that the list is not empty -- but there is no need to do that in your `insert()` function. Take a piece of paper and draw every step of the list manipulation -- I think it will really improve your understanding of what your code does wrong, and how Vlad's code works. – Snild Dolkow Oct 26 '15 at 06:52
@gates You could check whether head is equal to NULL in case when the new node was inserted in the tail of the list. – Vlad from Moscow Oct 26 '15 at 06:54
So you need not assign temp->next = NULL? if the list is empty? @Snild – gates Oct 26 '15 at 06:59
1

If the list is empty, then `head` is `NULL`. So `temp->next = head` actually assigns `NULL` to `temp->next` in that case! – Snild Dolkow Oct 26 '15 at 07:00
That's is what I'm missing :D Thanks Vlad and Snild – gates Oct 26 '15 at 07:01

Sourav Datta · Answer 2 · 2015-10-26T07:23:36.473

-1

this may be the insert function

void insert(int n)
{
     if(head==NULL)
     {
           head=(struct Node*)malloc(sizeof(struct Node));
           head->data=n;
           head->next=NULL;
     }
     else
     {
           struct Node *temp=(struct Node*)malloc(sizeof(struct Node));
           temp->data=n;
           temp->next=head;
           head=temp;
     }

}

edited Oct 26 '15 at 07:23

answered Oct 26 '15 at 06:49

Sourav Datta

83
6

Head is supposed to point to the first node, Why are you allocating data to it? – gates Oct 26 '15 at 06:53
head will be pointing to the 1st node. If I made a temp and allocated memory and at last head=temp, it would have the same effect – Sourav Datta Oct 26 '15 at 06:57
This does not the answer the question, "why does this not work?". Also, you're inserting at the end of the list, while the question says that @gates is trying to insert at the *beginning* of the list. This makes your implementation more complicated than it needs to be. – Snild Dolkow Oct 26 '15 at 07:03
ok sry I misunderstood the question. i'm changing accordingly – Sourav Datta Oct 26 '15 at 07:19

Why the following implementation overrides the elements/ or prints only one element

2 Answers2