I was learning link list concepts in C, as I was building a project that can create a single reverse order list, I am encountering segmentation code dumped again and again. Kindly help me find my errors
#include <stdio.h>
#include <stdlib.h>
typedef struct node{
int num;
struct node *next;
}node;
node* createll(node *head, node *error);
void print_list(node *head);
int main(void){
node *head = NULL;
node *error = NULL;
error->num = 1;
error->next = NULL;
head = createll(head, error);
print_list(head);
return 0;
}
node* createll(node *head, node *error){
int len;
printf("Size of list: ");
scanf("%d", &len);
for (int i = 0; i < len; i++) {
node *new_node = malloc(sizeof(node));
if (new_node == NULL) {
return error;
}
printf("Enter the data: ");
scanf("%d", &new_node->num);
new_node->next = NULL;
new_node = head;
head = new_node;
}
return head;
}
void print_list(node *head){
node *temp;
printf("Data stored is : ");
for (temp = head; temp != NULL; temp = temp->next){
printf("%d-->", temp->num);
}
printf("\n");
}
this is my code. I
Here are some of the issues:
error = NULL
followed by error->num = 1
is problematic. You should only dereference a pointer when you have allocated memory for it. As you have code to create a node elsewhere, you should not have this code here at all.
When you do new_node = head;
you lose the reference to the node you had just created with malloc
. So here you have a memory leak, and the loss of the number that the user had input. The following two points address this user input. Anyhow new_node = head
is wrong. You could prefix the new node into the linked list by setting new_node->next = head
, but this should better be done in a dedicated node-creating function.
Don't mix user-input logic with linked list logic. Keep the code for getting the user's input separated from the details of the linked list building logic. So I would perform all user-input logic in main
, where you will not have code that de-references nodes, but where you only call the linked-list related function to do the job. This also means that the loop -- for entering the numbers of the list -- should not be placed inside a function that is concerned with linked list building details.
Related to the previous point, you should not have scanf("%d", &new_node->num)
. Although this works, it mixes input concerns with node implementation details. Instead, read the input into a plain int
variable, and only create the corresponding node after that input has been made, inside a dedicated function that takes an int
as argument.
Following the previous points, you should not have a createll
function, but a create_node
function, which is only concerned with allocating the memory, and initialising the two members of the created node.
error
is a name that suggests it is some error-related item, but it is a node. The name seems not well-chosen.
When malloc
returns NULL
, then don't return from the function. There is no use in trying to continue the linked list building algorithm when you encounter such an error. Instead, output an error message and exit the program.
I would use Node
as name for the type, with an initial capital. This is a matter of opinion though.
Here is a possible version of your code that addresses the above points:
#include <stdio.h>
#include <stdlib.h>
typedef struct node {
int num;
struct node *next;
} Node;
Node* create_node(int num, Node* next);
void print_list(Node *head);
int main(void) {
Node *head = NULL;
int num, len;
// Perform input outside of functions that implement
// linked list operations (separation of concern).
printf("Size of list: ");
scanf("%d", &len);
for (int i = 0; i < len; i++) {
// Get the data into an int; not directly into a node member.
printf("Enter the data for node %d: ", i + 1);
scanf("%d", &num);
head = create_node(num, head);
}
print_list(head);
return 0;
}
// Separate the logic for creating one node and
// for building a multi-node linked list:
Node* create_node(int num, Node* next) {
Node *new_node = malloc(sizeof(Node));
if (!new_node) {
// Don't return, but exit
printf("Could not allocate memory for a new node.");
exit(1);
}
new_node->num = num;
new_node->next = next;
return new_node;
}
void print_list(Node *head){
printf("Linked list: ");
for (Node *temp = head; temp; temp = temp->next) {
printf("%d -> ", temp->num);
}
printf("null\n");
}
Note that prepending values into a linked list produces a linked list that has its values in reversed order. From your question I understand that this is what you wanted to happen.