0 
void SinglyLinkedList::removeBike(int c)
{
   Node* current = head;

   for(int i = 0; i < c; i++){         // traverse to desired node
       current = current -> next;
   } 

   if(current -> next != NULL && current -> prev != NULL){
        current -> prev -> next = current -> next;
        current -> next -> prev = current -> prev;
        delete current;                     //delete node
   }
   else if(current -> next == NULL){
      current -> prev -> next = current;
      delete current;
   }
   else if(current -> prev == NULL){
       current -> next -> prev = current;
       delete current;
   }    
   else 
       cout << "How did you make it this far?";

}

I'm fairly certain I'm not understanding something here, but research hasn't helped so far.

Logically it makes sense to me but I feel like this is too simple to work, which it isn't.

edited: updated the code minus input check and it breaks on me when I input any int. also I should probably rename the class to doubly linked list... just noticed that

Improve this question
5
  • 1
    Best way I've ever seen to figure out linked lists is draw the nodes on a piece of paper and then draw in all of the changes in linkage required step by step. Commented Feb 23, 2016 at 0:07
  • 1
    for(int i = 0; i < c; i++){ should be for(int i = 0; i < c || current != NULL; i++){. Commented Feb 23, 2016 at 0:27
  • 1
    && current != NULL works much better, for me... And then you'll have to figure out what to do when you wind up with a NULL current, after the loop. Commented Feb 23, 2016 at 0:27
  • @Josh Stop editing your question with tries to fix the code. Don't create moving targets that invalidate present answers. That's not useful. Commented Feb 23, 2016 at 0:29
  • its breaking on me on the first line of the first if statement. done with edits. Commented Feb 23, 2016 at 0:30

1 Answer 1

Reset to default
6

Nope, it's definitely not that simple.

First of all if the c parameter exceeds the size of the list, you'll blow up with a null pointer dereference, as your current pointer hits the null pointer on the last node, and blows past it.

Then, if c is 0 or the index of the last element of the list, either it's prev or it's next pointer, accordingly, will be null, and you'll also blow up with a null pointer dereference.

current -> prev -> next = current -> next;
current -> next -> prev = current -> prev;

If current is the first element in the list, current->prev will obviously null. If current is the last element in the list, current->next will obviously be null. Guess what happens when you attempt to dereference the null pointer?

Conclusions:

A) Check that the input parameter to the function is valid.

B) Provide for special handling when the node to be deleted is the first or the last node in the doubly-linked list.

EDIT: by popular request, someone wanted to know how I would fix this:

void SinglyLinkedList::removeBike(int c)
{
   Node* current = head;

   for(int i = 0; i < c; i++){
       if (!current)
           break;
       current = current -> next;
   } 

   if (!current)
           return; // Or throw an exception. Or return a bool false, maybe.

   if (current->prev)
       current -> prev -> next = current -> next;
   else
       head=current->next;

   if (current->next)
       current -> next -> prev = current -> prev;
   else
       tail=current->prev;  // Assuming that there's a tail, somewhere around here...
   delete current;                      //delete node

   // Maybe "return true;" here...
}
Improve this answer
Sign up to request clarification or add additional context in comments.

10 Comments

AH dunno how I messed it up that much. didnt even think about the first or last being the desired one. wow. appreciate the input! well i'll edit if I have further issues after ive done that.
No, no need to take this down. This is a valid question.
@SamVarshavchik A completely fixed code sample of the SinglyLinkedList::removeBike() function would be great at least.
"by popular request" I didn't recognize I'm such a popular person so far :-P, but THX. Though a nitpick // Maybe "return true;" here... The function return type is void actually.
@πάνταῥεῖ I think the return true was a suggestion, you would change the function signature. I agree as well, its nice to know if it has been removed or not.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.