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);
}

Answer 1

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.