linked list program is not functioning properly

Question

The given program is not showing all the elements of the linked list. I am having problem in identifying the error.

At first I initialized the head with a null value then made a temporary variable and assigned it an integer value and pointer to the next node. Then I made an another node named temp1 and linked it with the head. It will only be linked when "i" will be equal to 1.

Then equated temp1 to the next node and did the same.

//Linked list //Inserting the nodes.

#include <stdio.h>

struct node
{
    int n;
    struct node *next;
};

struct node *head;

int main ()
{
    int i, s, x, y;
    i = 0;
    struct node *temp;
    struct node *temp1;
    struct node *cur;
    head = NULL;
    scanf ("%d", &s);     //No. of nodes.
    while (i < s)
    {
        scanf ("%d", &x);
        if (head == NULL)
        {
            temp = (struct node *) malloc (sizeof (struct node));
            temp->n = x;
            temp->next = NULL;
            head = temp;
        }
        else
        {
            temp = (struct node *) malloc (sizeof (struct node));
            temp->n = x;
            temp->next = NULL;
            temp1 = temp;
            if (i == 1)
            {
                head->next = temp1;
            }
            temp1 = temp1->next;  //Assigning the next node.i.e. NULL value
        }
        i = i + 1;
    }
    cur = head;
    while (cur != NULL)
    {
        printf ("%d", cur->n);
        cur = cur->next;
    }
    return 0;
}

Unrelated to the problem, but don't cast the return value of malloc and prefer ptr = malloc(sizeof(*ptr)) to repeating the type. — Arkku
– Arkku, Commented Sep 6, 2019 at 16:13
Not related to your problem, but the y variable is never used — jmq
– jmq, Commented Sep 6, 2019 at 16:16
I'm not clear what you are using temp1 for since its value is never used. — jmq
– jmq, Commented Sep 6, 2019 at 16:22

Ratul Sharker · Accepted Answer · 2019-09-06 16:19:05Z

1

Check the following changed section

    {
      temp = (struct node *) malloc (sizeof (struct node));
      temp->n = x;
      temp->next = NULL;
      head = temp;
      temp1 = head;
    }
      else
    {
      temp = (struct node *) malloc (sizeof (struct node));
      temp->n = x;
      temp->next = NULL;
      temp1->next = temp;
      temp1 = temp1->next;  //Assigning the next node.i.e. NULL value
    }

Instead of relying on

   if (i == 1) {
      head->next = temp1;
   }

I assign head on temp1 while creating the head, which is meant to be happen only first time.

There were also some linkage issues in your else portion.

answered Sep 6, 2019 at 16:19

Ratul Sharker

8,0974 gold badges39 silver badges48 bronze badges

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

Comments

Arkku · Accepted Answer · 2019-09-06 16:36:44Z

1

You lose nodes beyond the first two, since you never link them to the list. Use meaningful names for variables: rename temp1 to tail and initialize it to NULL in the beginning. Then the loop body becomes:

if (scanf(" %d", &x) != 1) {
    // FIXME: handle error
}
temp = malloc(sizeof(*temp));
temp->n = x;
temp->next = NULL;
if (tail == NULL) {
    head = temp;
} else {
    tail->next = temp;
}
tail = temp;
++i;

(Untested.)

Rationale: You want to add new nodes to the end (tail) of the list. The easiest way is to keep track of the tail in an appropriately-named variable, and simply link every node to tail->next instead of convoluted logic like checking for the node count, etc. The only special case is the empty list, i.e., both head and tail are NULL, and the difference is just one line, so don't duplicate the whole block of code to set up the new node.

edited Sep 6, 2019 at 16:36

answered Sep 6, 2019 at 16:21

Arkku

42.3k10 gold badges68 silver badges88 bronze badges

1 Comment

Arkku Over a year ago

I would also revisit the names (and existence) of other variables, i.e., s is total_nodes, x could be n to match the field of struct node (although you could consider renaming that n to something like value?), i is node_count, and y is unnecessary and can be deleted. temp could also be called node. I find it helpful to first come up with a descriptive name for a variable, and if I can't there's probably something else I need to figure out first – adding something like temp1 is a good example of a sign to step back and rethink what you are doing. =)

Vlad from Moscow · Accepted Answer · 2019-09-06 16:43:17Z

For starters you have to include the header <stdlib.h>.

The problem is in this statement

temp1 = temp;

if i is not equal to 1 then after this statement

temp1 = temp1->next;

temp1 becomes equal to NULL.

So all other nodes are not added to the list because there is a cycling

temp1 = temp;
//...
temp1 = temp1->next;

Change the loop the following way

  while (i < s)
    {
      scanf ("%d", &x);
      if (head == NULL)
    {
      temp = (struct node *) malloc (sizeof (struct node));
      temp->n = x;
      temp->next = NULL;
      head = temp;
      temp1 = head;
    }
      else
    {
      temp = (struct node *) malloc (sizeof (struct node));
      temp->n = x;
      temp->next = NULL;
      temp1->next = temp;
      temp1 = temp;
    }
    i++;
    }

Pay attention to that you should declare variables in a block scope where they are used. Otherwise the program will be unreadable.

The approach you are using can be called as an Java approach.

In C the program can look much simpler. For example

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

struct node
{
  int n;
  struct node *next;
};

struct node *head;

int main( void )
{
    struct node **temp = &head;

    size_t n = 0;

    scanf( "%zu", &n );

    for ( size_t i = 0; i < n; i++ )
    {
        *temp = (struct node *) malloc ( sizeof ( struct node ) );

        int value = 0;

        scanf ( "%d", &value);

        ( *temp )->n = value;
        ( *temp )->next = NULL;

        temp = &( *temp )->next;
    }

    for ( struct node *cur = head; cur != NULL; cur = cur->next )
    {
        printf ("%d -> ", cur->n );
    }
    puts( "NULL" );

    return 0;
}

Its output might look like

1 -> 2 -> 3 -> NULL

Collectives™ on Stack Overflow

linked list program is not functioning properly

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related