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.

Unrelated to the crash, but you never initialize `list->data`, its value will be *indeterminate* and seemingly random. — Some programmer dude, Feb 27 '18 at 09:54
[Why you should not cast the result of `malloc()`](https://stackoverflow.com/questions/605845) — DevSolar, Feb 27 '18 at 09:55
As for your crash, please learn how to use a debugger, so you can step through the code line by line while monitoring variables and pointers etc. — Some programmer dude, Feb 27 '18 at 09:55
You do not `free()` your memory. Not all OS will reclaim un-`free()`d memory at the end of `main()`. — DevSolar, Feb 27 '18 at 10:01
What do you think that `list->data;` in the first loop does, why is that code there? — unwind, Feb 27 '18 at 10:11
@Someprogrammerdude that i have initialized to i. it got missed out while copying the code, sorry. — Gameatro, Feb 27 '18 at 10:54
Please edit your question to show the actual code, including initialization. — Some programmer dude, Feb 27 '18 at 11:13
Please [edit] your question to show us what kind of debugging you've done. I expect you to have run your [mcve] within Valgrind or a similar checker, and to have investigated with a debugger such as GDB, for example. Ensure you've enabled a full set of compiler warnings, too. What did the tools tell you, and what information are they missing? And read Eric Lippert's [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). — Toby Speight, Feb 27 '18 at 16:19
@TobySpeight there was no compiler warning as i was getting runtime error. that problem is solved btw, now i am dealing with the second problem i mentioned. — Gameatro, Feb 27 '18 at 23:39

Mathieu · Accepted Answer · 2018-02-27T11:34:48.100

1

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;
}

edited Feb 27 '18 at 11:34

answered Feb 27 '18 at 10:06

Mathieu

8,840
7
32
45

and what about the second problem I mentioned? – Gameatro Feb 27 '18 at 10:51
@purplepsypcho but doesnt just assigning list = NULL after the loop work? wouldnt that serve the same purpose? – Gameatro Feb 27 '18 at 12:44
well i got an easier soln for the problem, i simply put a i<=5 check in the loop, so the list=list->next is not executed for last iteration and the extra node is not created. – Gameatro Feb 28 '18 at 00:07

score 1 · Answer 2 · answered Feb 27 '18 at 10:06

1

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. ;-) )

answered Feb 27 '18 at 10:06

DevSolar

67,862
21
134
209

now its working. but what of the second problem. how to prevent the garbage element from creating at end? – Gameatro Feb 27 '18 at 10:56
@Gameatro: The second part of your question is unclear. What should `list = NULL` achieve? What "garbage" are you speaking of? – DevSolar Feb 27 '18 at 11:03
after the creation of list loop I have put list->next = NULL. it is creating an extra list with garbage val. but if i just write list=NULL, while printing with condition while(list!=NULL) it goes into infinity loop. – Gameatro Feb 27 '18 at 23:37

Vlad from Moscow · Answer 3 · 2018-02-27T12:51:55.020

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.:)

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

3 Answers3