Program crashes when trying to delete last node of a linked list

Question

I am creating a program in which you can and delete a node at any time. However, when I try to delete the last node in the list, the program crashes. Can somebody tell me what I am doing wrong?

void delete_node(Node **head_ref) {
    int position, counter = 0;
    Node *curr_node = *head_ref;

    if(*head_ref == NULL) {
        printf("No nodes to delete!");
        return;
    }

    printf("Enter position between 1 and %d: ", node_number);
    scanf("%d", &position);

    if(position == 1) {
        *head_ref = (*head_ref)->next;
        free(curr_node);
    } else if(position == node_number) {
        while(counter != position) {
            counter++;
            if(counter == position - 1)
                curr_node->next = NULL;
            curr_node = curr_node->next;
        }
    }

    printf("\nNode deleted successfully!\n");
    if( *head_ref == NULL )
        printf("\nLast node deleted!\n");
}

I am calling the function in main:

int main() {
    //... other part of the program
    delete_node(&head);
}

OT: You should do something about the variable names `node_num` and `node_number`. It is confusing. — Emanuel P, Apr 17 '21 at 19:04
You set `curr_node->next` to NULL but you never check if `curr_node` is NULL in your loop. — Emanuel P, Apr 17 '21 at 19:07
@EmanuelP I added if case if `curr_node == NULL` and this seems to be the problem but I don't understand how it becomes `NULL` in the first place — zaro, Apr 17 '21 at 19:13
`curr_node->next = NULL;` followed by `curr_node = curr_node->next`, and then the next iteration. — Emanuel P, Apr 17 '21 at 19:14
@EmanuelP Thank you very much. I think I should add a `prev_node` too, since `curr_node` is not enough — zaro, Apr 17 '21 at 19:17

score 0 · Answer 1 · answered Apr 17 '21 at 19:52

In the while loop, you do

while(counter != position) {
            counter++;

            if(counter == position - 1)
                curr_node->next = NULL;

            curr_node = curr_node->next;
        }

Let's take the example of a list with 3 nodes:
I'll use (°) to identify the Node pointed by curr_node.

first iteration:
initial status:
(°)--------------()--------------()
counter = 0

you only change variables.

second iteration:
initial status:
()--------------(°)--------------()
counter = 1

Now, the first thing your code do is increment counter. Then counter' value is now 2.
If statement results True, and now you actually delete the last node:
()--------------(°)--------------NULL
As last operation of second iteration, you do the assignment curr_node = curr_node->next; that results in cur_node = NULL. Finally, you end the second iteration with the following status:
cur_node = NULL
counter = 2

The problem is that you will start another iteration, because the ending condition is counter != position, that is still true. So you have a third iteration:
third iteration:
initial status:
()--------------()--------------NULL°
counter = 2
Note: curr_node points to NULL.
In the end of this iteration, you try to perform curr_node = curr_node->next;, that causes the crash because curr_node is actually NULL.

You can try to solve by changing the while condition in while (counter != position -1)

thank you for the explanation. I haven't seen that I am trying to change `curr_node` address after I changed the `next` pointer to it to `NULL`. I just added a `prev_node` and fixed it, but I think I could find a simpler fix as well. — zaro, Apr 17 '21 at 20:06

score 0 · Accepted Answer · answered Apr 17 '21 at 20:08

When counter == position - 1 you set current->next to NULL, but there will be one more iteration of the loop, and in that iteration current will be NULL and so you will run into an exception when you access current->next in that final iteration.

Some things you can do:

Introduce a prev_node reference, that follows curr_node one step behind.
You don't need to treat the deletion of the tail node as a separate case.
You don't need counter as you can decrease the value of position instead.
Don't forget to free the memory in the else case as currently you only do that when deleting the head node.
Don't forget to decrease the value of node_number when the deletion is successful.

Here is some updated code:

void delete_node(Node **head_ref) {
    int position;

    Node *curr_node = *head_ref;

    if(*head_ref == NULL) {
        printf("No nodes to delete!");
        return;
    }

    printf("Enter position between 1 and %d: ", node_number);
    scanf("%d", &position);

    if (position == 1) {
        *head_ref = (*head_ref)->next;
    } else { // All other positions can be treated in the same way
        Node *prev_node = curr_node; // Maintain a separate reference for previous node
        curr_node = curr_node->next;
        while (curr_node != NULL && position > 2) { // Can use position without counter
            prev_node = curr_node;
            curr_node = curr_node->next;
            position--;
        }
        if (curr_node == NULL) { // The input position was too great
            printf("\nThere is no node at that position!\n");
            return;
        }
        prev_node->next = curr_node->next;
    }
    free(curr_node);
    node_number--;  // Don't forget to keep this updated

    printf("\nNode deleted successfully!\n");
    if (*head_ref == NULL)
        printf("\nLast node deleted!\n");
}

NB: It would be better if you would not deal with I/O inside this function, and instead pass the position as a separate argument. Deal with I/O only in the main program code. The function could return a boolean or int indicating whether the node was found and deleted or not. The caller should deal with printing messages.

Thank you very much for the awesome explanation and tips! Your code is really better structured and cleaner. I just need to taka a few minutes to completely understand it. — zaro, Apr 17 '21 at 20:21
sorry, my bad. I understood everything, but I would have never ever come up with this idea. Just one question, is it better to add a while loop after entering the desired position, which will loop until the user enters a valid node position? Something like ```while(position < 1 || position > node_number) { printf("Enter position between 1 and %d: ", node_number); scanf("%d", &position); } ``` — zaro, Apr 18 '21 at 11:40
Yes, that would be a possibility. I would however keep that logic outside of the function, and deal with user input and output in the main program. That way you keep the "model" and the "view" concerns separate (see also [Separation of concern](https://en.wikipedia.org/wiki/Separation_of_concerns), [MVC](https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller) and related subjects). One issue though: the `while` condition does not deal with the case where `node_number` is zero. — trincot, Apr 18 '21 at 11:54
I was just going to ask you why is it better to deal with the I/O outside this function. Thank you for the resources! — zaro, Apr 18 '21 at 12:07

Program crashes when trying to delete last node of a linked list

2 Answers2