What is wrong with my code for Pairwise swapping of linked list in C?

Question

This is my code for pairswapping:

#include<stdio.h>
#include<stdlib.h>
struct Node{
     int data;
     struct Node *next; 
};
int main(){
struct Node *list, *head;
head = (struct Node *)malloc(sizeof(struct Node));
list = head;
int i;
for(i=0;i<6;i++){
    list->data=i;
    list->next = (struct Node *)malloc(sizeof(struct Node));
    list = list->next;
 }
list->next = NULL;
list = head;
while(list->next->next!=NULL){
    int temp = list->data;
    list->data = list->next->data;
    list->next->data = temp;
    list = list->next->next;
 }
 printf("Pair swapped list: ");
 while(head->next!=NULL){
       printf("%d\n",head->data);
        head=head->next;
  }
 return 0;
}

I have two problems. The main is that this code is giving a runtime error, I don't see why. and second thing is that if I use list = NULL instead of list->next = null and then while(list!=NULL) then it is causing infinite loop. and if the code is kept same, a garbage element is created at end. please help me.

Answer 1

For starters according to the C Standard the function main without parameters shall be declared like

int main( void )

In this loop

for(i=0;i<6;i++){
    list->data=i;
    list->next = (struct Node *)malloc(sizeof(struct Node));
    list = list->next;
 }
list->next = NULL;

there is allocated a redundant node with an inderterminate value of the data member data.

The condition in this while loop

while(list->next->next!=NULL){
    int temp = list->data;
    list->data = list->next->data;
    list->next->data = temp;
    list = list->next->next;
 }

is wrong and leads to undefined behavior due to this statement at the end of the loop

    list = list->next->next;

The condition should be written like

while ( list && list->next )

Using your approach the program can look the following way

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

struct Node
{
    int data;
    struct Node *next; 
};

int main(void) 
{
    struct Node *list, *head;
    const int N = 6;

    head = ( struct Node * )malloc( sizeof( struct Node ) );
    list = head;

    int i = 0;
    do
    {
        list->data = i;
    } while ( ++i < N && ( list = list->next = ( struct Node *)malloc( sizeof( struct Node ) ) ) );

    list->next = NULL;

    printf("Original list: ");
    for ( list = head; list; list = list->next )
    {
        printf( "%d ", list->data );
    }
    putchar( '\n' );

    list = head;

    while ( list && list->next )
    {
        int temp = list->data;
        list->data = list->next->data;
        list->next->data = temp;
        list = list->next->next;
    }

    printf("Pair swapped list: ");
    for ( list = head; list; list = list->next )
    {
        printf( "%d ", list->data );
    }
    putchar( '\n' );

    return 0;
}

Its output is

Original list: 0 1 2 3 4 5 
Pair swapped list: 1 0 3 2 5 4

You can add yourself checks that malloc(s) were successfull.

A more safe program can be written using the variable list of the type struct Node **.

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

struct Node
{
    int data;
    struct Node *next; 
};

int main(void) 
{
    const int N = 6;
    struct Node *head;
    struct Node **list;

    list = &head;

    for ( int i = 0; ( i < N ) && ( *list = ( struct Node *)malloc( sizeof( struct Node ) ) ); i++ )
    {
        if ( *list )
        {
            ( *list )->data = i;
            list = &( *list )->next;
        }
    }

    *list = NULL;

    printf("Original list: ");
    for ( list = &head; *list; list = &( *list )->next )
    {
        printf( "%d ", ( *list )->data );
    }
    putchar( '\n' );

    list = &head;

    while ( *list && ( *list )->next )
    {
        int temp = ( *list )->data;
        ( *list )->data = ( *list )->next->data;
        ( *list )->next->data = temp;
        list = &( *list )->next->next;
    }

    printf("Pair swapped list: ");
    for ( list = &head; *list; list = &( *list )->next )
    {
        printf( "%d ", ( *list )->data );
    }
    putchar( '\n' );

    return 0;
}

Take into account that you should free all allocated memory for the list.

And one more remark. The program does not swap nodes. It swaps values of the data member data of adjacent nodes.:)

Answer 2

The line causing problem is:

while(list->next->next!=NULL) {

You do not handle when list->next is NULL

So, just replace it with:

while(list->next != NULL && list->next->next != NULL) {

---

And for the second problem, as pointed by DevSolar, you must control better the list creation:

struct Node *list, *head, *last;
head = list = last = NULL; 

for (i=0; i<6; i++)
{
    /* create a node */
    list = malloc(sizeof *list);

    /* if head is not set, set it */
    if (NULL == head)
        head = list;

    /* fill new element */
    list->data = i;
    list->next = NULL;

    /* this new node is the child of the previous */
    if (NULL != last)
        last->next = list;     

     /* remember last created node */
    last = list;
}

Answer 3

You have 7 nodes -- the head, and the 6 you create in the loop.

Your while checks for list->next->next!=NULL, and at the end of the loop you move list like this: list = list->next->next;

This means your loop runs for:

• 1st node, swapped with 2nd
• 3rd node, swapped with 4th
• 5th node, swapped with 6th.

Then list is set to the 7th node (with list->next being NULL). And your while checks for list->next->next. As stated, list->next is NULL, list->next->next is an illegal memory access (dereferencing NULL).

(Deferring to purplepsycho for what you should do -- I don't copy from other people's answers. ;-) )