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;
}
puckerhoopAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

sunnycoderCommented:
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;
}
0
joselopesdacruzCommented:
when you delete de last node you put a NULL at the previous node ( but now the last one!)
0
puckerhoopAuthor Commented:
I am not necessarily deleting the last node... usually it is a MIDDLE node.
0
Ultimate Tool Kit for Technology Solution Provider

Broken down into practical pointers and step-by-step instructions, the IT Service Excellence Tool Kit delivers expert advice for technology solution providers. Get your free copy now.

sunnycoderCommented:
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
0
puckerhoopAuthor Commented:
null case has been taken care of - just not put in post
0
sunnycoderCommented:
good ... what about when target is in first node and list has 1 element ?
0
puckerhoopAuthor Commented:
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;
}}
0
puckerhoopAuthor Commented:
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.
0
sunnycoderCommented:
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->next = temp->next;          //there was error in this statement
 cout << "BOOK DELETED" << endl;
 free(temp);
 return true;                    
 }                        
else    
before = before->next;              
}                        
return false;
}}
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
puckerhoopAuthor Commented:
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!
0
sunnycoderCommented:
>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
0
puckerhoopAuthor Commented:
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?
0
puckerhoopAuthor Commented:
I am not deleting the last element... i am deleting the 3rd from the last.
0
sunnycoderCommented:
> 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
0
puckerhoopAuthor Commented:
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.
0
sunnycoderCommented:
could be a problem with your display function
0
puckerhoopAuthor Commented:
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.
0
puckerhoopAuthor Commented:
i have tested my display function as a solo function and it works fine.
0
sunnycoderCommented:
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
0
puckerhoopAuthor Commented:
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.
0
puckerhoopAuthor Commented:
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.
0
sunnycoderCommented:
>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 ?
0
puckerhoopAuthor Commented:
the only global is "head" which is declared as private in my llistBooks class.
0
puckerhoopAuthor Commented:
bookNodePtr is a typedef for bookNode (which is the struct).
0
puckerhoopAuthor Commented:
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.
0
sunnycoderCommented:
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
0
sunnycoderCommented:
>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
0
puckerhoopAuthor Commented:
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!
0
sunnycoderCommented:
cheers :o)
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Programming

From novice to tech pro — start learning today.

Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.