Unexpected output in a linked list using C

Question

I've just started learning C and am fairly a beginner. Today in school we learned linked list and I was able to put up a code...which thankfully is running without errors.

#include<stdio.h>
#include<stdlib.h>
struct node
{
    int data;
    struct node *next;
}*head;//*temp;
void create(struct node **h,int num)
{
    int i;
    struct node *temp=*h;
    for(i=0;;i++)
    {
        if(i>=num)
        break;
        temp->data=i;
        temp->next=malloc(sizeof(struct node));
        temp=temp->next;
    }
    temp->next=NULL;
}
    void display(struct node **h)
{   
    struct node *temp=*h;
    while(temp->next!=NULL)
    {
        printf("%d->",temp->data);
        temp=temp->next;
    }
    printf("\b\b  \b\b");
}
void append_end(struct node **h,int val)
{
    struct node *temp=*h,*temp1;
    //printf("'%d'",val);
    while(temp->next!=NULL)
    temp=temp->next;
    temp1=malloc(sizeof(struct node));
    temp1->data=val;
    temp1->next=NULL;
    temp->next=temp1;
}
void free_list(struct node **h)
{
    struct node *temp=*h,*tail;
    while(temp->next!=NULL)
    {
        tail=temp;
        temp=temp->next;
        free(tail);
    }
    h=NULL;
}
int main()
{
    head=malloc(sizeof(struct node));
    int i,num;
    scanf("%d",&num);
    create(&head,num);
    //display(&head);
    append_end(&head,5);
    append_end(&head,6);
    display(&head);
    /*temp=head;
    while(temp->next!=NULL)
    temp=temp->next;
    printf("%d",temp->data);*/
    free_list(&head);
    return 0;
}

The expected output should be 0->1->2->3->5->6 for input of 4

But instead I'm getting 0->1->2->3->(some garbage value)->5

I'll be glad if someone could point out my error(s), and/or link to any article that might help me in understanding the topic clearly.

Thanks in advance.

You should depend less on global variables. Also, you're over-relying on pointers. You shouldn't have to be dealing with pointers to pointers with what you're doing. — Wug
– Wug, Commented Aug 21, 2012 at 17:29
Thanks for the speedy reply Wug...for global variables I'll just declare the structure in main...and I'll look up to make the code less reliant on pointers. But when using functions here doesn't it make more sense to use pointers to pointers?? — user1614886
– user1614886, Commented Aug 21, 2012 at 17:40
I don't see any reason to, since in every one you just dereference it one level before using them. — Wug
– Wug, Commented Aug 21, 2012 at 17:48
I think your other problem, adding 5 and 6 to the end of the list, is really coming from the create() method..this is just from eyeballing it, but I believe your code causes the ->next reference for the last member to actually point to a new structure rather than null..so all the comparisons of temp->next==null don't work as you expect... — David W
– David W, Commented Aug 21, 2012 at 18:06
Okay I've figured out one of my mistakes When displaying while loop should be temp!=NULL instead of temp->next!=NULL silly me :P But now the output is 0->1->2->3->(some garbage value)->5->6 for an input of 4 — user1614886
– user1614886, Commented Aug 21, 2012 at 18:08

Wug · Accepted Answer · 2012-08-21 18:13:04Z

2

I've refactored your code. You can see it here: http://ideone.com/nZ55i

Your code was ugly, so I tweaked it into my own style. I suggest you find a style you like, that's easy for you to read.
Moved declaration of head into main method (used to be a global variable)
Functions that took pointer-to-pointer types for no reason have been modified to take pointers.
You used malloc(sizeof(struct node)) in at least 3 places, so I just up and made a function that does it for you.
Added a typedef to the declaration of the node struct, you can declare instances of it now with just node derp;
create function uses a for loop with an empty condition and a break, refactored to use a proper for loop with no break
The garbage value was caused by a bug in your create function that prevented it from writing the value to the last node it created. To fix it, I moved stuff around so it always assigns the data field but doesn't allocate a new node if it's at the end
Failure to write the last item in the list was caused by a bug in the display function that caused it to terminate before displaying the last node.

If this is a homework question and you turn in my code, I will find you, drive to your house, and shoot a rotten potato through your bedroom window with a cannon.

answered Aug 21, 2012 at 18:13

Wug

13.3k5 gold badges36 silver badges57 bronze badges

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

1 Comment

user1614886 Over a year ago

Thanks for the code...yes yours is easy on the eyes...I'hope to find my style sometimes soon. I know my problem now. PS Don't worry its not even remotely related to homework.

David W · Accepted Answer · 2012-08-21 18:12:20Z

1

Okay, I started to put this in the comments, but putting code in comments will get brickbats :). This is not tested, and there are myriad ways to approach it (given I've diagnosed the problem correctly :) ), but one way that might work to fix your create method is offered as follows: (Note that I've simplified your loop, just using the normal termination condition for the 'for' loop construct, and eliminated the 'if..break' within the loop as its no longer needed. The "->next" member will always be initialized to a new member if there's one left to create, otherwise its NULL, so we don't need the ending assignment:

void create(struct node **h,int num)
{
    int i;
    struct node *temp=*h;
    for(i=0;i<num;i++)
    {
        temp->data=i;
        if (i==(num-1))
           temp->next=NULL;
        else
           temp->next=malloc(sizeof(struct node));

        temp=temp->next;
    }
}

answered Aug 21, 2012 at 18:12

David W

10.2k38 silver badges60 bronze badges

3 Comments

user1614886 Over a year ago

Thanks David...this create function makes sense and now I know what my problem was.

r4. Over a year ago

Just to get things a bit clearer. Are you saying 'replace void create with this', everything else is 'rather ok' OR are you just commenting on the create function?

David W Over a year ago

Just suggesting an alternative way to implement the "create" method that, I think, fixes a small bug in the OP's version and tries to clarify the code a little. The refactored example by @Wug is very nice, so the organization it demonstrates mitigates the need for too much more comment from me in the broader vein of "code cleanup" or "organization."

Collectives™ on Stack Overflow

Unexpected output in a linked list using C

2 Answers 2

1 Comment

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related