1

I tried to create a program to add elements to a linked list. The elements consist of name and age. But it fails to add without giving me any error. Could you please show me my mistake?

#include "stdio.h"
#include "stdlib.h"
#include "string.h"

#define MAX 9999

struct data {
    char name[MAX];
    int age;
    struct data *next;
};

void pushHead(struct data **head, struct data **tail, char name[], int age) {
    struct data *node = (struct data *)malloc(sizeof(struct data));

    strcpy(node->name, name);
    node->age = age;

    if (*head == NULL) {
        node = *head;
        node = *tail;
        node->next = NULL;
    } else {
        node->next = *head;
        *head = node;
    }
}

void view(struct data *head) {
    struct data *curr = head;
    if (curr == NULL)
        printf("No Data\n");
    else {
        while (curr != NULL) {
            printf("%s(%d)\n", curr->name, curr->age);
            curr = curr->next;
        }
    }
}

int main(int argc, char const *argv[]) {
    struct data *head = NULL;
    struct data *tail = NULL;

    pushHead(&head, &tail, "Felix", 19);
    view(head);
    return 0;
}

Output : No Output

My code is working when I put the head on global scope (by changing all the functions to work globally), but when I try to put the head in main scope it doesn't work.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
Bone Shaw
  • 21
  • 1
  • 2
    `node = *head;` and `node = *tail;` – user3386109 Mar 08 '20 at 07:25
  • 3
    Note that the dot `.` and arrow `->` operators bind very tightly indeed and should not be surrounded by spaces on either side. – Jonathan Leffler Mar 08 '20 at 07:28
  • The code should not work either with `head` in global scope, but you might have made some other changes that fix the bug. – chqrlie Mar 08 '20 at 08:07
  • You may benefit from looking at the `add` function in [Singly-Linked List](https://pastebin.com/5MPLU4wB) and further from providing both a `head` and `tail` pointer for efficient insertions at the end of the list by using a wrapper struct [Singly-Linked List w/head/tail ptr](https://pastebin.com/R2AewR3A) allowing you to create as many lists as you need. – David C. Rankin Mar 08 '20 at 09:55

1 Answers1

3

In pushHead(), you are doing:

    node = *head;
    node = *tail;

this end up assigning NULL to node pointer because *head and *tail both are NULL. Note that this is a memory leak as your program loose the memory reference which the node pointer is holding. Instead, you should do

    *head = node;
    *tail = node;

Some suggestions:

  • For storing name in the list node, you are taking buffer of size 9999 (MAX macro) which is (IMO) very large. I believe, a buffer of size 256 is more than enough for this purpose. Or, you can also have buffer of exact size required for storing name by allocating the memory dynamically to it. For this , you have to take a char * member instead of char array for name and allocate memory to it dynamically based on size of name parameter of pushHead() and in this case, you need to make sure to free it explicitly when deleting the list nodes.
  • When using strcpy() to copy string, make sure that destination buffer is large enough to contain the source string to avoid overflows.
  • Follow good programming practice. Always check malloc return and ensure to free the allocated memory once you are done with it.
  • Do not cast the malloc return.
  • To include standard library header files use <>, i.e. #include "stdio.h" -> #include <stdio.h>, check this.
H.S.
  • 11,654
  • 2
  • 15
  • 32