Start Free Trial

asked on

Delete node from linked list

I am only leaving this question out for a couple of hours...
I need to delete a node in a linked list (the node has more than one piece of data). My current function seems to be removing this node, but damages the list, so that further deletes are not working.

bool llistBooks::deleteB(string &target)
{
bookNoce Ptr before = head;
bookNodePtr temp = before->next;
for (before; before != NULL; before = before->next)
for (temp; temp != NULL; temp = temp->next)
{
if (temp->isbn == target)
{
cout << temp->isbn << "temp" << endl;
cout << before->isbn << "before" << endl;
temp = before;
before = temp->next;
before->next = temp->next;
cout << "BOOK DELETED" << endl;
return true;
}
else
before = before->next;
}
return false;
}

bool llistBooks::deleteB(string &target)
{
bookNoce Ptr before = head;
bookNodePtr temp = before->next;

for (temp; temp != NULL; temp = temp->next)
{
if (temp->isbn == target)
{
cout << temp->isbn << "temp" << endl;
cout << before->isbn << "before" << endl;
// temp = before;
// before->next = temp->next;
before->next = temp->next;
cout << "BOOK DELETED" << endl;
free (temp); //if it was dynamically allocated .. else comment it out
return true;
}
else
before = before->next;
}
return false;
}

joselopesdacruz

when you delete de last node you put a NULL at the previous node ( but now the last one!)

ASKER

I am not necessarily deleting the last node... usually it is a MIDDLE node.

there are a few problems with your code ....
1. What happens if your target is present in the first node ?
2. What happens if there are are 0 or 1 elements in your linked list ?

I have corrected the logical error related to deletion in my previous post ... however, I have not added/modified code for these case as this sounds like homework ...

These are the directions you need to be thinking of if you want this function to work for all cases ... The code I corrected in my previous post will work for a list with several nodes and deleting a middle node.

If you need further help, post here

ASKER

null case has been taken care of - just not put in post

good ... what about when target is in first node and list has 1 element ?

ASKER

the cleaned up code works better, but it still cuts off my linked list at this point.
Any thoughts on how to avoid this separation would be appreciated.

full function with modifications:
-----------------------------------
bool llistBooks::deleteB(string &target)
{
if (empty() )
{
cout << "List is already empty." << endl;
return false;
}
else
{
bookNodePtr before = head;
bookNodePtr temp = before->next;
for (temp; temp != NULL; temp = temp->next)
{
if (temp->isbn == target)
{
cout << temp->isbn << "temp" << endl;
cout << before->isbn << "before" << endl;
before = temp->next;
cout << "BOOK DELETED" << endl;
free(temp);
return true;
}
else
before = before->next;
}
return false;
}}

ASKER

I did not do a case for target in the first node, as I am loading from file a whole series of nodes prior to calling this function.
It is a good point though, I will work on it.

ASKER CERTIFIED SOLUTION

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

ASKER

before->next needs to point to what temp->next was pointing to... got it.
yet, somehow it is still cutting of my list?!
i don't get it!

>yet, somehow it is still cutting of my list?!
thats strange ... try printing temp->next in each iteration (print the address) ... it needs to be NULL in the last iteration to cut off the list ..

check if you are deleting the last element of the list

ASKER

as I walk thru it step by step and put outputs to display, I see that I am now attaching properly, but am somehow dropping the node that is AFTER the new attachment. How can this be?

ASKER

I am not deleting the last element... i am deleting the 3rd from the last.

> but am somehow dropping the node that is AFTER the new attachment. How can this be?
you drop it inside this function or outside it ? My guess is outside it ... stuff in this function looks ok

ASKER

As I walk thru with "temp" immediately in the function. It does seem to be working properly, but when I output all to screen, the last one is droppoing off.
When I call delete again.... I get garbage.
I am still working on it.

could be a problem with your display function

ASKER

could it possibly have to do with "temp"... I use this in other functions... should it be freed or not freed at the end of each function. I had 'deleted' it at the end of some functions.

ASKER

i have tested my display function as a solo function and it works fine.

If it is local variable then other functions have no effect ... However, if you have declared it globally, then you need to be very very careful

better approach is to keep temporary variables local

ASKER

would it be best to use the same temporary node name in each function? As I comment out the deletes & frees, the program runs better.

ASKER

i had thought that I had set it up as a temporary. I have been calling bookNodePtr in all functions, just like you saw in this function.

>would it be best to use the same temporary node name in each function?
does not matter at all as long as they are local ... for the compiler it is nothing more than a symbol representing a variable

>As I comment out the deletes & frees, the program runs better.
you mean speed up ?

ASKER

the only global is "head" which is declared as private in my llistBooks class.

ASKER

bookNodePtr is a typedef for bookNode (which is the struct).

ASKER

taking out the deletes and frees from the end of the function:
it does not speed it up, the program runs cleaner... i don't get kicked out as often.
ie. if I have freed "temp", I am getting kicked out of compiler after the function runs. If I comment it out, it will go on to the next function.

then it should be just fine ... there is some logical error in one of your functions ...

call all your functions one by one and print the values in between ... you will get garbage after erroneous function ... else try stepping through the code and locate the function/statement where you get an error and then try to backtrace to the origin of the error

>if I have freed "temp", I am getting kicked out of compiler after the function runs
use free/delete if and only if the nodes were added dynamically (using malloc or new) ...

try commenting all free() and then run it ... if it runs perrfectly, then try uncommenting them one by one until you get an error ...
looks like you may be freeing a valid node .. or freeing without copying/changing addresses

ASKER

Actually, I think I have found the problem... it is another function that is called after delete. I thought it was working fine, but it seems that it was not.
Thank you!

cheers :o)