-2
\$\begingroup\$

For deleting a node in linked list I used 3 steps: 1)Finding the previous node of the node to be deleted. 2)Changing the next of the previous node.(to know what is next see my code it's fully explained). 3)Freeing up memory of deleted node. But in place of data to be deleted ,0 is coming.I don't know what the problem is??

#include <stdio.h>
#include <stdlib.h>

// A linked list node
struct Node
{
    int data;
    struct Node *next;
};

/* Given a reference (pointer to pointer) to the head of a list
and an int, inserts a new node on the front of the list. */
void push(struct Node** head_ref, int new_data)
{
    struct Node* new_node = (struct Node*) malloc(sizeof(struct Node));
    new_node->data = new_data;
    new_node->next = (*head_ref);
    (*head_ref) = new_node;
}

/* Given a reference (pointer to pointer) to the head of a list
and a key, deletes the first occurrence of key in linked list */
void deleteNode(struct Node **head_ref, int key)
{struct Node* temp=*head_ref;
    if(temp->data==key)
    {
        *head_ref=temp->next;
        free(temp);
        return;
    }
    if(temp==NULL)
    {
        return;
    }
    struct Node* prev=*head_ref;
    while(temp->data!=key&&temp!=NULL)
    {
        temp=temp->next;
    }
    prev=temp;
    temp=temp->next;
    free(prev);
    return;


}

// This function prints contents of linked list starting from 
// the given node
void printList(struct Node *node)
{
    while (node != NULL)
    {
        printf(" %d ", node->data);
        node = node->next;
    }
}


int main()
{
    /* Start with the empty list */
    struct Node* head = NULL;

    push(&head, 7);
    push(&head, 1);
    push(&head, 3);
    push(&head, 2);

    puts("Created Linked List: ");
    printList(head);
    deleteNode(&head, 1);
    puts("\nLinked List after Deletion of 1: ");
    printList(head);
    return 0;
}
\$\endgroup\$
3
  • \$\begingroup\$ Hey, welcome to Code Review! This question does not match what this site is about. Code Review is about improving existing, working code. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way! Please see our help center for more information. \$\endgroup\$
    – Graipher
    Aug 18, 2018 at 16:44
  • \$\begingroup\$ @Graipher Oh I am really sorry for it,will take care from next time. \$\endgroup\$
    – Hanoi
    Aug 18, 2018 at 17:01
  • \$\begingroup\$ No worries, we all had to start somewhere. And learn by making mistakes :-) \$\endgroup\$
    – Graipher
    Aug 18, 2018 at 19:15

1 Answer 1

-1
\$\begingroup\$

Fixing your code really fast:

struct Node* prev=*head_ref;
while(temp->data!=key&&temp!=NULL)
{
    prev = temp;
    temp=temp->next;
}
prev->next = temp->next;
free(temp);

You need to set the previous pointer in the loop to know what was the previous node. In your code you don't have this information. When you know what is the previous node, you can easily skip the node with the found key (prev->next = temp->next;).

This solution is still not perfect, what if the item is not present in the list? It is going to crash in this scenario, because you handle founding the key, and not founding the same way.

I would prefer to solve this with recursion:

typedef struct Node Node;
void recursion_remove(Node* node, Node* prev, int key){
    if(node == NULL)
        return; //Item is not found

    // Found the key
    if(node->data == key){
        //Node can't be NULL so we can write node->next
        //node->next can be null, it means the list end, it is not a problem
        prev->next = node->next;
        free(node)
        return;
    }

    recursion_remove(node->next,node,key);
}
void delete_node(Node ** head, int key){
    Node* head_node = *head;
    if(head_node ->data == key){
        Node* temp = head_node->next;
        free(head_node);
        *head = temp;
    }
    recursion_remove(head_node->next,head_node,key);
}

This way all the different cases are differentiated. You only need to modify the head if you are removing the first element (double indirection), but when you are removing other elements you don't need to modify the head

\$\endgroup\$
3
  • \$\begingroup\$ I have solutions to this problem but I want to know what's incorrect in my code. No,but after the loop runs we have got temp pointing at node to be deleted and then we set prev to temp so now prev also pointed to that node and then we are changing where the temp points that is to the next node after the node to be deleted,so that information is there. \$\endgroup\$
    – Hanoi
    Aug 18, 2018 at 13:10
  • \$\begingroup\$ Yes exactly what you say. Your prev is pointing to the same place where temp points which is the node with the key. This means you haven't got a pointer, which points to the previous node. Now you are setting the current node next pointer to the next node, which is NULL because it is the end of the list, this is why you see 0. Do you need a diagram to understand it? \$\endgroup\$
    – Ryper
    Aug 18, 2018 at 13:14
  • 1
    \$\begingroup\$ Are you trying to say that since there is no node pointing at the previous node and I am setting something to that node's field which can't be accessed or done without using a pointer to that node? \$\endgroup\$
    – Hanoi
    Aug 18, 2018 at 13:24

Not the answer you're looking for? Browse other questions tagged or ask your own question.