0

This is my complete code. There seems to be some problem when I try to concatenate two linked lists and then merge (in increasing order) those two lists in the next line.

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

struct Node {
    int data;
    struct Node *next;
} *first = NULL, *second = NULL, *third = NULL, *fourth = NULL; //global pointers

void create1(int a[], int n) {
    first = (struct Node *)malloc(sizeof(struct Node));
    first -> data = a[0];
    first -> next = NULL;
    int i;
    struct Node *t, *last;
    last = first;

    for (i = 1; i < n; i++) {
        t = (struct Node *)malloc(sizeof(struct Node));
        t -> data = a[i];
        t -> next = NULL;
        last->next = t;
        last = t;
    }
}

void create2(int a[], int n) {
    second = (struct Node *)malloc(sizeof(struct Node));
    second->data = a[0];
    second->next = NULL;
    struct Node *t, *last;
    last = second;
    for (int i = 1; i < n; i++) {
        t = (struct Node *)malloc(sizeof(struct Node));
        t->data = a[i];
        t->next = NULL;
        last->next = t;
        last = t;
    }
}

void Display(struct Node *p) {
    while (p != NULL) {
        printf("%d ", p->data);
        p = p->next;
    }
}
void concatenation(struct Node *p, struct Node *q) {
    third = p;
    while (p->next != NULL) {
        p = p->next;
    }
    p->next = q;
}

void mergeLL(struct Node *p, struct Node *q) {
    struct Node *last = NULL;
    if (p->data < q->data) {
        fourth = p;
        last = p;
        p = p->next;
        fourth->next = NULL;
    } else {
        fourth = q;
        last = q;
        q = q->next;
        fourth->next = NULL;
    }

    while (p && q) {
        if (p->data < q->data) {
            last->next = p;
            last = p;
            p = p->next;
        } else {
            last->next = q;
            last = q;
            q = q->next;
        }
        last->next = NULL;
    }
    if (p!=NULL) last->next = p;
    else last->next = q;
}

int main(int argc, char const *argv[])
{
    int a[] = {3,4,5,7,10,15,20,25,25,30};
    int b[] = {2,8,9};
    create1(a, 10); Display(first); printf("\n");
    create2(b, 3);  Display(second); printf("\n");
    mergeLL(first, second);  Display(fourth); printf("\n");
    concatenation(second, first);  Display(third); printf("\n");
    
    return 0;
}

Here, in main function, when I have merge and then concatenate, the concatenated list output keeps on going till infinity.

However, when I change the order of the lines like this:

concatenation(second, first);  Display(third); printf("\n");
mergeLL(first, second);  Display(fourth); printf("\n");

Then my merged list is not being printed. Both of those statements are working individually. Why not together?

I went over each function multiple times but all of them seem correct to me. I honestly don't even know what to do to try to fix it. Commenting out the concate statement in main function gave me the correct result. They just don't give me a proper result when the statements come one after the other

P.S: I tried asking chatGPT the same thing and it just gave me a bunch of nonsense. This may be a stupid doubt but please help.

user438383
  • 5,716
  • 8
  • 28
  • 43
user271914
  • 11
  • 1
  • OT: But if you have two functions that does the exact same thing, then perhaps it should be only *one* function? – Some programmer dude Aug 21 '23 at 16:34
  • No, my merge function is to make a list such that all the data in each node are ordered in increasing order. Concatenate is just to put one list after the other. – user271914 Aug 21 '23 at 16:36
  • As for your problem, use a [*debugger*](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) to step through the code line by line while monitoring variables and their values. At the same time, draw out the lists using pencil and paper. Draw nodes as squares, links and other pointers as arrows. When you modify a pointer in the code, erase and redraw the arrow on the paper. Does the drawing you make make any sense? – Some programmer dude Aug 21 '23 at 16:37
  • I mean `create1` and `create2`. Why are those different functions? Why can't they be one function which returns the created list? And global variables are generally a bad habit. – Some programmer dude Aug 21 '23 at 16:37
  • When you merge or concatenate the lists, you change one or more of the links in the lists themselves. Then there is no original `first` list or `second` list; modifying the links makes it so that at least one of `first` or `second` is no longer linked the way it used to be, and at least one of them points into the other. Then an attempt to concatenate or merge those original lists corrupts the data because passing it `first` and `second` does not pass it proper separate lists. If you want to do multiple operations on `first` and `second`, you need to make copies of the lists. – Eric Postpischil Aug 21 '23 at 16:38
  • @Someprogrammerdude I'll fix that and I'll also use only one create. I have tried tracing my code with a pencil, but didn't find anything wrong. Maybe I made a mistake in tracing the code. Is there something obviously wrong with the logic that I missed? – user271914 Aug 21 '23 at 16:40
  • @EricPostpischil I see that makes sense. – user271914 Aug 21 '23 at 16:43
  • So your question is answered? If not, can you update your question, dealing with what has been commented above, and explaining what is still the problem? One more comment: the idea of a merge is that you end up with one, merged list. Since you have one merged list, there is nothing that should be concatenated. – trincot Aug 21 '23 at 18:45
  • @trincot Yea I learnt where I made mistakes. The comments were helpful and I will check out the answer below as well. Once I modify the links and merge them, then yeah there is nothing more to concatenate. In hindsight it seems obvious. When I asked it wasn't. Thanks a lot! My doubt is clarified. – user271914 Aug 21 '23 at 21:16
  • @Someprogrammerdude I will definately try not to use global variables. Thank you! – user271914 Aug 21 '23 at 21:17

2 Answers2

2

For starters it is a bad idea to declare global variables when moreover there is no reason to do so

struct Node {
    int data;
    struct Node *next;
} *first = NULL, *second = NULL, *third = NULL, *fourth = NULL; //global pointers

In this case your functions will depend on global variables.

Declare all required pointers in main.

Almost all your functions are unsafe and can invoke undefined behavior when for example used pointers are null pointers. You need to check pointers whether they are null pointers.

In this case you could define for example only one function create the following way

struct Node * create( const int a[], size_t n ) 
{
    struct Node *head = NULL;
    
    for ( struct Node **current = &head; n && ( *current = malloc( sizeof( struct Node ) ) ) != NULL; --n )
    {
        ( *current )->data = *a++;
        ( *current )->next = NULL;
        current = &( *current )->next;
    }

    if ( n != 0 )
    {
        while ( head != NULL )
        {
            struct Node *tmp = head;
            head = head->next;
            free( tmp );
        }
    }

    return head;
}    

And in main you can write

int main( void )
{
    int a[] = { 3, 4, 5, 7, 10, 15, 20, 25, 25, 30 };
    int b[] = { 2, 8, 9 };

    struct Node *first  = create( a, sizeof( a ) / sizeof( *a ) );
    struct Node *second = create( b, sizeof( b ) / sizeof( *b ) );

    //...

Pay attention to that you should follow some convention in naming functions. Either names of all functions should start with low-letters as the function create or with upper letters as your function Display.

The renamed function display can look the following way

FILE * display( const struct Node *head, FILE *fp ) 
{
    for ( ; head != NULL; head = head->next )
    {
        fprintf( fp, "%d -> ", head->data );
    }

    fputs( "null", fp );

    return fp;
}

and be called in main like

fputc( '\n', display( first, stdout ) );
fputc( '\n', display( second, stdout ) );

You need to write one more function that will free all the allocated memory for lists.

As for your problem then after calling for example the function mergeLL the both lists first and second are mixed each with other. Just output the lists first and second after calling the function and you will see

3 -> 4 -> 5 -> 7 -> 8 -> 9 -> 10 -> 15 -> 20 -> 25 -> 25 -> 30 -> null
2 -> 3 -> 4 -> 5 -> 7 -> 8 -> 9 -> 10 -> 15 -> 20 -> 25 -> 25 -> 30 -> null

As you can see if to call after that the function concatenation

concatenation(second, first);

the data member next of the node with value 30 will be changed pointing to the node with value 3. As a result neither data member next of the result list will be equal to NULL.

The both functions mergeLL and concatenation either should add nodes from one list to another list making one of the lists an empty list or create a new list from the source lists making the bouth source lists empty or allocating dynamically nodes for the created list anew leaving the source lists unchanged.

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
  • Thanks for that! I learned how to write a proper create linked list function. Printing both the linked lists after merging has suddenly made all the pieces in my mind fit together. I mean now my doubt itself doesn't make sense. But still this was very useful. Thanks for your patience in explaining this and thanks to everyone's support in the comments as well. My doubt is resolved. – user271914 Aug 21 '23 at 21:22
  • @user271914 No at all. You are welcome.:) – Vlad from Moscow Aug 21 '23 at 21:24
0

As this is a very common task for beginners I will left below some arguments and a complete C code example and expected output. I think it may help others.

Using your functions these code loops forever:

    concatenation(second, first);
    Display(third);
    printf("\n");
    concatenation(second, first);
    Display(third);
    printf("\n");

So you can see concatenation() is not working.

This also loops:

    mergeLL(first, second);
    Display(fourth);
    printf("\n");

    mergeLL(first, second);
    Display(fourth);
    printf("\n");
  • So mergeLL() is also problematic.

As you were told by Eric and others global values are problematic, much more if they points to allocated stuff.

  • As you write code that malloc things also you must write code to free these things. This is basic for a good life with C

A node is not a (linked) list

This is the linked list as in your code:

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

But this is just a node. It is even named Node.

  • A node is not a list. A list is not a node.
  • A linked list is a --- possibly empty --- set of nodes.
  • The nodes points to or contains data. In your case the data is a single int value.
  • A list is a collection of nodes --- java naming --- or a container of nodes --- C++ naming.
    • Your code does not even have a struct list. But it should have. As close as your model is to the data the easier your code becomes.

create1 and create2 are not the same. But ARE

In your code they differ by the list address, first or second, both global values.

Good for you that there are only 2 lists and not 200, or you will end up with a very strange piece of code. Here you see the reason of pointer and arguments exists...

a safer approach

I will let here an example

a list

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

typedef struct
{
    unsigned size;
    Node*    head;
    Node*    tail;
} List;

So we now have List and it is a list of nodes Node. And both are names created by typedef. And List has metadata: keeping size updated makes total sense, as you will see in the example.

creating lists vc inserting data

    List* create();
    List* destroy();

    int   insert(int data, List* list);
    int   display(List* list, const char* title);

Here we have code to create and destroy List. insert() is a separate function, since insertion it is not part of the creation of a list.

And display() has an optional title, very convenient in the testing phase...

And create and destroy does just the expected.

destroy() returns NULL and it is very handy in order to invalidate the list pointer at the same time the list is destroyed: no invalid pointer is possible if you write

        List* my_list = create();
        // ...
        my_list = destroy(my_list);

merge() and concat() returns new lists

    List* concat(List* one, List* other);
    List* merge(List* one, List* other);

concat and merge returns new lists. This is the safe way to go: no globals, all data is copied.

The list in this example

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

typedef struct
{
    unsigned size;
    Node*    head;
    Node*    tail;
} List;

List* create();
List* destroy(List* toGo);
List* concat(List* one, List* other);
int   display(List* list, const char* title);
int   insert(int data, List* list);
int   insert_sorted(int data, List* list);
List* merge(List* one, List* other);

new here is

    int   insert_sorted(int data, List* list);

that just inserts data in the sorted (ascending) position. It is the simplest way to merge lists.

This declarations are the code that usually goes to a header file and gives a view of what you are implementing. If you write this as xlist.h and the code in xlist.c then you can write as many versions of test code or main.c as you need, whithout even recompling xlist.c. Nothing new, but convenient.

Note that there are no globals and no functions returning void. Functions return 0 for success or error codes. Or pointers to data.

simple versions of create, destroy and display

List* create()
{
    List* one = malloc(sizeof(List));
    if (one == NULL) return NULL;
    one->size = 0;
    one->head = NULL;
    one->tail = NULL;
    return one;
};

List* destroy(List* gone)
{
    if (gone == NULL) return NULL;
    for (unsigned i = 0; i < gone->size; i += 1)
    {
        Node* p = gone->head->next;
        free(gone->head);
        gone->head = p;
    }
    free(gone);
    return NULL;
};

int display(List* list, const char* title)
{
    if (title != NULL) printf("%s", title);
    if (list == NULL)
    {
        printf("\nList does not exist\n");
        return 0;
    }
    printf("\nList size: %u\n  [ ", list->size);
    Node* p = list->head;
    for (unsigned i = 0; i < list->size; i += 1)
    {
        printf("%d ", p->data);
        p = p->next;
    }
    printf("]\n\n");
    return 0;
};

Since we have size it is easy to free all memory allocated for Node and List items.

Note that a program like

int main(void)
{
    List* one = create();
    display(one, "testing: Empty LIST");
    one = destroy(one);

    display(one, "testing: No list");
    one = destroy(one);

    return 0;
}

Can already run and it should print

testing: Empty LIST
List size: 0
  [  ]

testing: No list
List does not exist

But it works already freeing all memory allocated.

versions of insert() and insert_sorted

int insert(int data, List* list)
{   // insert at last element
    if (list == NULL) return -1;
    Node* p = malloc(sizeof(Node));
    p->data = data;  // data copied to node
    p->next = NULL;  // inserted at tail
    if (list->size == 0)
    {
        list->head = list->tail = p;
        list->size              = 1;
        return 0;
    }
    list->tail->next = p;
    list->tail       = p;
    list->size += 1;  // count it in
    return 0;
}

int insert_sorted(int data, List* list)
{
    // insert in position according to value
    if (list == NULL) return -1;
    Node* p = malloc(sizeof(Node));
    p->data = data;  // data copied to node
    p->next = NULL;  // inserted at tail
    if (list->size == 0)
    {
        list->head = list->tail = p;
        list->size              = 1;
        return 0;
    }
    Node* pos  = list->head;
    Node* prev = NULL;
    while (pos->data < data)
    {
        if (pos == list->tail)
        {  // new element is the greatest
            pos->next  = p;
            list->tail = p;
            list->size += 1;
            return 0;
        }
        prev = pos;
        pos  = pos->next; // advance
    };  // end while()
    // 'pos' is the insertion point
    if (pos == list->head)
    {   // new head
        list->head = p;
        p->next    = pos;
        list->size += 1;
        return 0;
    }
    // insert before 'pos' 
    prev->next = p;
    p->next   = pos;
    list->size += 1;  // count it in
    return 0;
}

To insert in ascending order of data takes no more than 25 lines. Code would be simpler if we had also pointer to previous node in Node struct: single linked lists are more complicated and not the reverse...

the original example using this mode

int main(void)
{
    List* one = build_list(
        10, (int[]){3, 4, 5, 7, 10, 15, 20, 25, 25, 30});
    display(one, "'one'");

    List* other = build_list(3, (int[]){2, 8, 9});
    display(other, "'other'");

    List* both = merge(one, other);
    display(both, "merge(one,other)");
    both = destroy(both);

    both = concat(one, other);
    display(both, "concat(one,other)");

    one   = destroy(one);
    other = destroy(other);
    both  = destroy(both);
    return 0;
}

output of this example

'one'
List size: 10
  [ 3 4 5 7 10 15 20 25 25 30 ]

'other'
List size: 3
  [ 2 8 9 ]

merge(one,other)
List size: 13
  [ 2 3 4 5 7 8 9 10 15 20 25 25 30 ]

concat(one,other)
List size: 13
  [ 3 4 5 7 10 15 20 25 25 30 2 8 9 ]

build_list helper function

List* build_list(unsigned N, const int data[])
{
    List* L = create();                      // new list
    if ((N == 0) || data == NULL) return L;  // for safety
    for (unsigned i = 0; i < N; i += 1) insert(data[i], L);
    return L;
}

This 3-line function just builds a List with the supplied vector of int

Complete C code

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

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

typedef struct
{
    unsigned size;
    Node*    head;
    Node*    tail;
} List;

List* create();
List* destroy(List* toGo);
List* concat(List* one, List* other);
int   display(List* list, const char* title);
int   insert(int data, List* list);
int   insert_sorted(int data, List* list);
List* merge(List* one, List* other);

List* build_list(unsigned N, const int data[]);

int main(void)
{
    List* one = build_list(
        10, (int[]){3, 4, 5, 7, 10, 15, 20, 25, 25, 30});
    display(one, "'one'");

    List* other = build_list(3, (int[]){2, 8, 9});
    display(other, "'other'");

    List* both = merge(one, other);
    display(both, "merge(one,other)");
    both = destroy(both);

    both = concat(one, other);
    display(both, "concat(one,other)");

    one   = destroy(one);
    other = destroy(other);
    both  = destroy(both);
    return 0;
}

List* build_list(unsigned N, const int data[])
{
    List* L = create();                      // new list
    if ((N == 0) || data == NULL) return L;  // for safety
    for (unsigned i = 0; i < N; i += 1) insert(data[i], L);
    return L;
}

List* create()
{
    List* one = malloc(sizeof(List));
    if (one == NULL) return NULL;
    one->size = 0;
    one->head = NULL;
    one->tail = NULL;
    return one;
};

List* destroy(List* gone)
{
    if (gone == NULL) return NULL;
    for (unsigned i = 0; i < gone->size; i += 1)
    {
        Node* p = gone->head->next;
        free(gone->head);
        gone->head = p;
    }
    free(gone);
    return NULL;
};

int display(List* list, const char* title)
{
    if (title != NULL) printf("%s", title);
    if (list == NULL)
    {
        printf("\nList does not exist\n");
        return 0;
    }
    printf("\nList size: %u\n  [ ", list->size);
    Node* p = list->head;
    for (unsigned i = 0; i < list->size; i += 1)
    {
        printf("%d ", p->data);
        p = p->next;
    }
    printf("]\n\n");
    return 0;
};

List* concat(List* one, List* other)
{
    if (one == NULL) return NULL;
    if (other == NULL) return NULL;
    List* res = create();
    // copy 1st
    Node* p = one->head;
    for (unsigned i = 0; i < one->size; i += 1)
    {
        insert(p->data, res);
        p = p->next;
    };
    // copy 2nd
    p = other->head;
    for (unsigned i = 0; i < other->size; i += 1)
    {
        insert(p->data, res);
        p = p->next;
    };
    return res;
};

int insert(int data, List* list)
{  // insert at last element
    if (list == NULL) return -1;
    Node* p = malloc(sizeof(Node));
    p->data = data;  // data copied to node
    p->next = NULL;  // inserted at tail
    if (list->size == 0)
    {
        list->head = list->tail = p;
        list->size              = 1;
        return 0;
    }
    list->tail->next = p;
    list->tail       = p;
    list->size += 1;  // count it in
    return 0;
}

int insert_sorted(int data, List* list)
{
    // insert in position according to value
    if (list == NULL) return -1;
    Node* p = malloc(sizeof(Node));
    p->data = data;  // data copied to node
    p->next = NULL;  // inserted at tail
    if (list->size == 0)
    {
        list->head = list->tail = p;
        list->size              = 1;
        return 0;
    }
    Node* pos  = list->head;
    Node* prev = NULL;
    while (pos->data < data)
    {
        if (pos == list->tail)
        {  // new element is the greatest
            pos->next  = p;
            list->tail = p;
            list->size += 1;
            return 0;
        }
        prev = pos;
        pos  = pos->next;  // advance
    };                     // end while()
    // 'pos' is the insertion point
    if (pos == list->head)
    {  // new head
        list->head = p;
        p->next    = pos;
        list->size += 1;
        return 0;
    }
    // insert before 'pos'
    prev->next = p;
    p->next    = pos;
    list->size += 1;  // count it in
    return 0;
}

List* merge(List* one, List* other)
{  // returns a new sorted list with
    // all values in the 2 input lists
    if (one == NULL) return NULL;
    if (other == NULL) return NULL;
    List* mrg = create();  // new list
    Node* p   = one->head;
    for (unsigned i = 0; i < one->size; i += 1)
    {
        insert_sorted(p->data, mrg);
        p = p->next;
    }
    p = other->head;
    for (unsigned i = 0; i < other->size; i += 1)
    {
        insert_sorted(p->data, mrg);
        p = p->next;
    }
    return mrg;  // result in new list
};
arfneto
  • 1,227
  • 1
  • 6
  • 13