Thread: Help me finding the error in my delete node function

I have written a delete node function to delete a node in a linked list by the index number. Here is the code:

Code:

void delete_node(struct node **start,int index_no)
{    int counter=0;
      struct node *current=*start, *prev=NULL;//Current holds start and prev holds previous node of current
      
      while(current->next!=NULL)
       {
          current=current->next; //Traversing the list
		prev->next=current;// updating the prev node accordingly
	     counter++;//counter to keep track of the index
		if(counter==index_no)//if user input and node no matches except the head node
		{
                printf("\n Hurrah! We caught the node!!");
                
                prev->next=current->next; //Making sure the prev node holds the address of the next to current ndoe
			free(current); //deleting current
           }

}
}

Please note that I know if the head node turns out to be the indexed position, there is no way to delete that node in the current coding. But please after correcting the present node add that part of the code separately.

You have set *prev to NULL and then you do prev->next = current.

So, you request a member named "next" in NULL. You see the error here, don't you?

It also seems that you did good in the procedure, before writing the code, which is taking a paper and a pen and actually draw your list and the actions your functions does.

Do that again and try to delete the first node. You will find out what is happening.

Repeat for last node. In general, when writing code for manipulating a list, check how they react in these three situations:

• Manipulate the first node.
• Manipulate a node in the middle.
• Manipulate the last node.

Also notice that when you do

Code:

prev->next=current;

, assuming that prev does not point anywhere. It probably points to garbage. Only the "next" points to something.

Originally Posted by Matticus

Did you have a specific question?

Some things I noticed:

- Why are you passing a pointer to pointer to start? If that's supposed to be the head node, you shouldn't have to modify it in this function (considering the code you provided isn't intended to be able to delete the head node)

- "prev" isn't pointing to anything at line 8, so how can you assign a value to the member of a non-existent struct?

- If you successfully free current, then you shouldn't be using that pointer anymore - but that is what your code does, when it returns to the top of the loop. Perhaps you intended to put a break or return in the body of that if statement?

It would also be a good idea to:

- add more error checking to the function (e.g. make sure the pointers passed aren't already NULL)

- return a value, so you know if the delete was successful or not

everything you noticed and assumed are correct. Points to be noticed:

1) I intend to delete the head node later,so I used pointer to pointer to start.

2) As you suggested, I added a break in the if loop.

Now please tell me,

1)How to make "prev" node to point the just before node of "current". I intend to do that, but unable to code correctly.

2) And how to return a value to check if the deletion was successful. Please provide a code snippet if possible.