Linked list Node Insert to Front - all content value changing each time I add a value

Question

I am making a singly linked list whose node has a string and a pointer to next node. I have written a function to insert to front of the linked list. The problem is that whenever I insert a new value to the linked list, it changes the value of all the nodes. I don't know where I'm going wrong. Please help. Here is the code

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

typedef struct Elem {
char *word;
struct Elem *next;
} Elem;

void printList (Elem **list)
{
if(!(*list)){
    printf("List is empty");
    return ;
}
Elem *curr;
curr = *list;
while(curr)
{
    printf("%s -- ",(curr)->word);
    curr = (curr)->next;
}
}

void insert_in_front(Elem **list, char *value)
{
if(!*list)
{
    printf("List is empty... creating first node\n");
    (*list) = (Elem*) malloc(sizeof(Elem));
    (*list)->word = value;
    (*list)->next = NULL;
    return ;
}

printf("The word in list is %s\n",(*list)->word);

Elem *curr = (Elem*) malloc(sizeof(Elem));
if(!curr)
    exit(-1);
curr->word = value;
curr->next = (*list);
printf("the address of curr is : 0x%x\n",curr);
(*list) = curr;
printf("the address of list is : 0x%x\n",(*list));
}

int main(void)
{
Elem *newList;
newList = NULL;
char inp[15];
while(1)
{
    printf("Enter the string : ");
    scanf("%s",&inp);
    printf("input is %s",inp);
    printf("\nthe address of newList is : 0x%x\n",newList);


    insert_in_front(&newList, &inp);
    printf("the address of newList is : 0x%x\n",newList);
    printList(&newList);
    printf("the address of newList is : 0x%x\n",newList);
    printf("\n");
}
return 0;
}

You can copy paste the code to run. The output is as follows : Please excuse the debug messages. I just wanted to see if the pointer is pointing to new location after each insert.

Enter the string : hello
input is hello
the address of newList is : 0x0
List is empty... creating first node
the address of newList is : 0x251b010
hello -- the address of newList is : 0x251b010

Enter the string : world
input is world
the address of newList is : 0x251b010
The word in list is world
the address of curr is : 0x251b030
the address of list is : 0x251b030
the address of newList is : 0x251b030
world -- world -- the address of newList is : 0x251b030

Enter the string : testing
input is testing
the address of newList is : 0x251b030
The word in list is testing
the address of curr is : 0x251b050
the address of list is : 0x251b050
the address of newList is : 0x251b050
testing -- testing -- testing -- the address of newList is : 0x251b050

Enter the string :

Thanks in advance!

[Don't cast the return value of `malloc()`.](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/605858#605858) — , Jun 09 '13 at 06:43
@H2CO3 stop giving bad advice. There is no good reason not to cast it (forgetting to include malloc is not a good reason) and ability to compile as c++ is a big benefit. — xaxxon, Jun 09 '13 at 06:48
@xaxxon It's not bad advice. 1. Compiling C code as C++ should not even happen. C code is compiled with a C compiler, just like one does not try to compile Java with a Pascal compiler. 2. Forgetting to include headers can indeed happen (we are humans, not computers, after all), and a compiler warning is useful, so it **is** a good reason. 3. The biggest problem with the cast is that it **decreases readability,** which no one ever wants. — , Jun 09 '13 at 06:51
@abat I didn't post that as an answer, just as a comment. It won't solve your problem, I just suggested that because it's bad. By the way, did you try running your program in a debugger? — , Jun 09 '13 at 06:51
@xaxxon - It is good advice - besides you should use `new` in C++ world — Ed Heal, Jun 09 '13 at 06:52
it's not as good of advice as people make it out to be and having that be the first thing that people get told each time really makes people focus on something that is incredibly minor and debatable. — xaxxon, Jun 09 '13 at 07:00
@H2CO3 @xaxxon this `char *p; p = (char *) malloc(strlen(s)+1);` from k & r c programming book... I don't understand why they have used it when it's wrong... — pinkpanther, Jun 09 '13 at 07:07
Even I didn't know typecasting the return of malloc in C is wrong — abat, Jun 09 '13 at 07:18
@pinkpanther the language has evolved over time. Some people only care about the most recent versions. It is clearly not required anymore, but at one point it was the way to do things. — xaxxon, Jun 09 '13 at 07:18
@abat that's because it's not "wrong", it's just not considered best practice by the group-think at StackOverflow — xaxxon, Jun 09 '13 at 07:19
@xaxxon I just read [here](http://computer-programming-forum.com/47-c-language/a9c4a586c7dcd3fe.htm), that k & r book is wrong in recommending that.... have you read this before? — pinkpanther, Jun 09 '13 at 07:20
@pinkpanther Regardless, I have no problem with casting. The downsides are highly contrived, and ability to compile with a c++ compiler is a big win in my book. — xaxxon, Jun 09 '13 at 07:27

xaxxon · Accepted Answer · 2013-06-09T07:01:51.640

4

The problem is that you're setting everything to a single variable -- inp

You're not making copies, you're making each node point to that same address. When you change it in subsequent scanf calls, you're changing what every node points to.

Use strdup or something to make a copy of inp and assign ->word to the new copy. For example, you could say:

    insert_in_front(&newList, strdup(inp));

Don't forget to free it later!

Tip: there's no need to pass a double pointer to printList. Since you don't intend to change anything, passing a double pointer only gives you the ability to do the wrong thing and actually change the head pointer to your list outside the scope of the function. It also makes the code harder to understand inside the function. Change it to a single pointer, get rid of all the dereferences, and you can even get rid of the curr and just use list to iterate, since it's just a copy of the pointer, not the top-level actual list pointer.

edited Jun 09 '13 at 07:01

answered Jun 09 '13 at 06:54

xaxxon

19,189
5
50
80

Now this is more constructive. +1. – Jun 09 '13 at 06:55
That was damn fast. Thanks xaxxon – abat Jun 09 '13 at 07:05
1

@abat and I was even arguing with h2co3 at the same time :) – xaxxon Jun 09 '13 at 07:06
:D But a new insight to the typecasting stuff – abat Jun 09 '13 at 07:24

Linked list Node Insert to Front - all content value changing each time I add a value

1 Answers1