First attempt at making linkedlist in c

Question

I started learning c and wanted to challenge myself by making a linked list. I am stil new to the concept of pointers and pointers to pointers, so there might be something in my code that I could have done differently, but hey, I'm still learning. I would like to hear any advice or tips so I can improve my programming skills.

Code:

#include <stdio.h>
#include <stdlib.h>
#define TRUE 1
#define FALSE 0
typedef struct node{
 int val;
 struct node * next;
} node_t;
/** Prints out all the elements **/
void print_nodes(node_t * head)
{
 if(head == NULL)
 printf("Head is null");
 node_t * current = head;
 printf("Node values\n");
 while(current != NULL)
 {
 printf("%d\n",current->val);
 current = current->next;
 }
 printf("\n");
}
/** Removes the current head node **/
void pop(node_t ** head_node)
{
 printf("val: %d\n",(*head_node)->val);
 node_t * new_head = (*head_node)->next;
 *head_node = new_head;
}
/** Removes a node at he given index **/
void remove_node_at(node_t ** head_node,int index)
{
 node_t * current = *head_node;
 node_t * node_after_del;
 node_t * prev_node;
 int run = TRUE;
 int counter = 0;
 if(index == 0)
 {
 pop(head_node);
 run = FALSE;
 }
 while(run == TRUE)
 {
 if(counter == index)
 {
 if(current->next == NULL)
 {
 prev_node->next = NULL;
 free(current);
 }
 else
 {
 node_after_del = current->next;
 prev_node->next = node_after_del;
 free(current);
 }
 run = FALSE;
 }
 if(current->next != NULL)
 {
 prev_node = current;
 current = current->next;
 }
 else
 {
 run = FALSE;
 printf("Node to delete not found\n\n");
 }
 counter++;
 }
}
/** Free up all the memory allocated by the nodes**/
void clear_nodes(node_t * head_node)
{
 node_t * current = head_node;
 int run = TRUE;
 while(run == TRUE)
 {
 node_t * temp = current;
 if(temp->next != NULL)
 {
 current = temp->next;
 }
 else
 {
 run = FALSE;
 }
 temp = NULL;
 free(temp);
 }
 head_node = NULL;
 free(head_node);
}
/** Pushes a node to the front **/
void push_front(node_t ** heade_node, int val)
{
 node_t * new_head = malloc(sizeof(node_t));
 new_head->val = val;
 new_head->next = *heade_node;
 *heade_node = new_head;
}
/** Adds a new node at the end **/
void push_end(node_t * head_node,int val)
{
 node_t * current = head_node;
 int run = TRUE;
 while(run == TRUE)
 {
 if(current->next == NULL)
 {
 run = FALSE;
 }
 else
 current = current->next;
 }
 node_t * next = malloc(sizeof(node_t));
 if(next == NULL)
 exit(1);
 next->val = val;
 next->next = NULL;
 current->next = next;
}
/** Inserts a node at the given position, if its to big it gets added at the end **/
void insert_node_after(node_t * head_node, int insert_location, int val)
{
 node_t * current = head_node;
 int run = TRUE;
 int counter = 0;
 while(run)
 {
 if(current->next == NULL) run = FALSE;
 if(counter == insert_location)
 {
 node_t * new_node = malloc(sizeof(node_t));
 if(new_node == NULL)
 exit(1);
 new_node->val = val;
 new_node->next = current->next;
 current->next = new_node;
 run = FALSE;
 }
 if(current->next == NULL)
 {
 /** Adds the node at the end if we reached the end **/
 node_t * new_node = malloc(sizeof(node_t));
 if(new_node == NULL)
 exit(1);
 current->next = malloc(sizeof(node_t));
 current->next->val = val;
 current->next->next = NULL;
 run == FALSE;
 }
 else
 {
 current = current->next;
 }
 counter++;
 }
}
int main()
{
 node_t * head = NULL;
 head = malloc(sizeof(node_t));
 if(head == NULL)
 return 1;
 head->val = 0;
 head->next = NULL;
 for(int i = 1; i < 13; i++)
 {
 push_end(head,i);
 }
 push_front(&head,17);
 print_nodes(head);
 remove_node_at(&head,0);
 pop(&head);
 pop(&head);
 print_nodes(head);
 insert_node_after(head,1,199);
 print_nodes(head);
 clear_nodes(head);
 return 0;
}

• Cris Luengo: I think pop(&head) leaks memory.
• swingBoris: @CrisLuengo could you explain why? And how it should be done then?
• Cris Luengo: Because you're no longer referencing the old head node, and there’s no free. I might see this wrong, haven’t combed through the code in fine detail. You might want pop to call remove_node_at(...,0) or something similar to have only one bit of code that removes a node.
• pacmaninbw: @CrisLuengo is correct,, there is a memory leak there. One thing I left out of my review is there should be a delete_node() function to complement the create_node() function.
• pacmaninbw: @vnp is also correct in the comments under my review, several of the functions in this implementation should return values that indicate the success or failure of the action and error handling should be handled at a higher level. Neither exit nor setjmp/longjmp is necessary in this program. You need to figure out a strategy for handling errors.

Answer 1

BUG Report
The function void insert_node_after(node_t * head_node, int insert_location, int val) contains at least one bug, which is a typo and may contain more bugs in the then clause of the third if statement. The typo is

 run == FALSE;

which should be an assignment statement with a single equal sign. The full context is shown below:

 if (current->next == NULL)
 {
 /** Adds the node at the end if we reached the end **/
 node_t * new_node = (node_t *) malloc(sizeof(node_t));
 if (new_node == NULL)
 exit(1);
 current->next = (node_t *) malloc(sizeof(node_t));
 current->next->val = val;
 current->next->next = NULL;
 run == FALSE;
 }
 else
 {
 current = current->next;
 }

Report Errors
The code checks for a failed call to malloc(size_t) in some instances and exits, however, it never reports that malloc failed so anyone using the program would wonder why the program failed. Prior to calling exit(1) it might be better to report the error to stderr. Note that the error check is not performed in void push_front(node_t ** head_node, int val), and it is missing after the third malloc in void insert_node_after(node_t * head_node, int insert_location, int val).

Prefer setjmp and longjmp Over exit(1)
Some programs such as operating systems should never call exit(int status). Many more programs may need to clean up things, such as closing files, detaching from databases or deleting memory. It might be better to call setjmp() in main() and then execute a longjmp if malloc(size_t) fails. This is discussed in greater detail in this stackoverflow question.

Use System Defined Symbolic Constants Rather than Magic Numbers
The code already references stdlib.h, which contains the system defined constants EXIT_SUCCESS and EXIT_FAILURE. Since these constants are always defined in stdlib.h the code will be more portable if they are used, and it also makes the code more readable:

main()
{
 ...
 return EXIT_SUCCESS;
}
exit(EXIT_FAILURE);

Basic Principles When Writing Code
One of the earliest principles all programmers learn is Don't Repeat Yourself, usually shortened to DRY code. Basically anytime you have code that repeats it would be better to put it into a loop or a function where the code is reused.

A second principle that should be taught early but is part of more complex programming is the Single Responsibility Principle which states that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated within the function, module or class. This reduces the size of functions which allows functions to be more readable, maintainable and easier to debug. It also allows the programmer to debug the code only once rather than multiple times.

The code to create and fill a node is repeated 5 times, it might be better to write a function that creates and fills a node:

node_t * create_node(int val, node_t *next_node)
{
 node_t * new_node = malloc(sizeof(*new_node));
 if (new_node == NULL)
 {
 fprintf(stderr, "malloc for new_node failed.\n");
 exit(EXIT_FAILURE); // replace with call to longjmp here.
 }
 new_node->val = val;
 new_node->next = next_node;
 return new_node;
}

In the function above sizeof(*new_node) was used for the size of the malloc() this allow the programmer to edit once to change the type of a variable rather than edit in two places this can prevent some programming errors when maintaining code.

Keep It Simple
The function void print_nodes(node_t * head) contains a simpler loop to traverse the linked list. This might be better than using the variable run that is used in the other traversals of the linked list.

• vnp: The setjmp/longjmp recommendation is seriously off, especially given the skill level of the OP.
• pacmaninbw: @vnp I can definitely see your point about the skill level, what would you suggest?
• vnp: @pacmaninbw Report an error, just as you said, and let the caller decide. *jmp has its use case, but it is not here.