Keep printing the last element in the linked list

Question

I created a standard linked list in C. It asks the user to input a number, and program end if user input #. If the user inputs anything else the program will stop.

The problem is that my program runs forever and prints the normal list at first then keeping print the last element of the linked list. Hope someone could tell me where did I made mistake.

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

typedef struct node {
    int data;
    struct node *next;
} NodeT;

void freeLL(NodeT *list) {
    NodeT *p, *temp;
    p = list;
    while (p != NULL) {
        temp = p->next;
        free(p);
        p = temp;
    }
}

void showLL(NodeT *list) {
    NodeT *temp = list;
    temp = temp->next;
    printf("Done. The list is ");
    printf("%d", temp->data);
    temp = temp->next;
    //iterate the entire linked list and print the data
    while (temp != NULL) {
        printf("-->");
        printf("%d", temp->data);
        temp = temp->next; 
    }
}

NodeT *joinLL(NodeT *list, int v) {
    NodeT *current = list;
    NodeT *head;
    head->data = v;
    head->next = NULL;
    while (current->next != NULL) {
        current = current->next;
    }
    current->next = head;
    return head;
}

int main() {
    int data;
    NodeT *list = NULL;
    list = (NodeT *)malloc(sizeof(NodeT));
    printf("Enter a number: ");
    if (scanf("%d", &data) != 1) {
        printf("Done. ");
    } else {
        printf("Enter a number: ");
        joinLL(list, data);
        while (1 == scanf("%d", &data)) { 
            printf("Enter a number: ");
            joinLL(list, data);
        }
        showLL(list);
        freeLL(list);
    }
    
    return 0;
}

I believe the problem is in the joinLL function which add a new node at the end of the linked list.

After `list = malloc(...)`, the value of `list->next` is not initialized. Later code (incorrectly) relies on it being set to NULL. — William Pursell, Oct 03 '21 at 10:32
You should reduce the redundancy and initialize list to NULL, and allocate space for it by calling `joinLL`. You'll need to change the API to make that happen, but that is cleaner than having to allocated it as a special case. — William Pursell, Oct 03 '21 at 10:33

ipnah · Answer 1 · 2021-10-03T13:08:10.827

1

Your program keeps running because of a memory access error, you did not allocate memory for your head(you set a pointer, but use it directly without initializing it)

Change to this may solve the problem:

head=(NodeT*)malloc(sizeof(NodeT));
if(NULL==head)
{
   // failed : do something...
   return NULL;
}
head->data=v;
head->next=NULL;

When I just tested it, I found that there was another problem:

    list = (NodeT*)malloc(sizeof(NodeT));

malloc will not be initialize your list, so the value that your list->next initially points to is uncertain.

in c, malloc does not need to be cast.

edited Oct 03 '21 at 13:08

answered Oct 03 '21 at 02:23

ipnah

125
7

I am not a C programmer (not trying to), but casting a malloc is **redundant** (https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) except you know what you are doing. If you want to make sure the reader know what the value is, or make the C portable to C++, then it is maybe acceptable. – FaranAiki Oct 03 '21 at 07:03
@FaranAiki emm,this is my habit... (I often need to make sure that my code is compatible with C++) – ipnah Oct 03 '21 at 08:32
1

add an explanation to why the cast is used. I usually, for now, use `// Unnecessary casting.` and an explanation to why I cast is in the header/comment. – FaranAiki Oct 03 '21 at 08:56

score 1 · Accepted Answer · answered Oct 03 '21 at 13:28

The problem is you do not allocate elements in joinLL: only a single element in allocated in main().

You should instead always allocate the element in joinLL and update the head pointer from the return value.

Similary, freeLL should take a pointer to head and set it to NULL for consistency.

Here is a modified version:

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

typedef struct node {
    int data;
    struct node *next;
} NodeT;

void freeLL(NodeT *p) {
    while (p != NULL) {
        NodeT *temp = p->next;
        free(p);
        p = temp;
    }
}

void showLL(const NodeT *list) {
    NodeT *p = list;
    printf("The list is ");
    if (p == NULL) {
        printf("empty");
    } else {
        printf(" %d", temp->data);
        while ((p = p->next) != NULL) {
            printf("--> %d", temp->data);
        }
    }
    printf("\n");
}

NodeT *joinLL(NodeT *head, int v) {
    NodeT *newp = malloc(sizeof(*p));
    NodeT *current;

    if (newp == NULL) {
        fprintf(stderr, "allocation failure\n");
        exit(1);
    }
    newp->data = v;
    newp->next = NULL;
    if (head == NULL) {
        return newp;
    }
    for (current = head; current->next != NULL; current = current->next)
        continue;

    current->next = newp;
    return head;
}

int main() {
    NodeT *list = NULL;
    for (;;) {
        int data;
        printf("Enter a number: ");
        if (scanf("%d", &data) != 1) {
            printf("Done. ");
            break;
        }
        list = joinLL(list, data);
    }
    showLL(list);
    freeLL(list);
    return 0;
}

Keep printing the last element in the linked list

2 Answers2