1

My problem is my delete method isn't deleting the node I want to delete and giving me a infinite loop.

public void delete(String name){
      Node current = head;
      boolean checker = false;
         while(current != null && current.name != name && checker != true){
            try{
               if(current.name.equals(name)){
                  Node p = current.previous;
                  Node q = current.next;
/*The code can somehow get through only above this line, below here its not anymore*/
                  p.next = q;
                  q.previous = p;
                  System.out.println("Item successfully deleted.");
                  checker = true;
               }else if(!current.name.equals(name) && current == tail){
                  System.out.println("Item not found.");
               }
               current = current.next;
            } catch(NullPointerException e){}          
         }
   }

Im here to asking for a hint or tip about my problem (Sorry for my bad english)

4
  • 1
    do you get item not found in the output? Commented Dec 24, 2020 at 12:38
  • not sure if this is the sole issue, but in the loop header, you compare Strings with ==. Commented Dec 24, 2020 at 12:39
  • Your catch program has an empty try-block. Do not do this unless you really know what you're doing. At the very least add e.printStackTrace() for debugging purposes. Commented Dec 24, 2020 at 15:53
  • This would be a great time to learn to use a debugger. Commented Dec 24, 2020 at 15:54

4 Answers 4

1

You are checking if you have reached the end of the list current == tail but not breaking out of it. You can add a break statement inside your else if.

Other than that, you are using == to compare strings. I'm not sure why you added that there and it can be removed. Also, you must (almost always) never catch a NullPointerException.

Sign up to request clarification or add additional context in comments.

Comments

1

"infinite loop" means your loop condition is incorrect, you are not making progress in each iteration, or there is a cycle your data. You use both current == null and current == tail to signify that it's the last element. Choice one way. Suggest you rewrite your loop condition to only deal with iteration, and have a conditional with a break if you have a match in the body:

for(current = head; current; current = current.next) {
   if(current.name.equals(name)) {
     if(current == head)
        head = current.next
     else
        current.previous.next = current.next; 

     if(current == tail)
        tail = current.previous;
     else
        current.next.previous = current.previous;

     break;
   }
   // if tail.next is not initialized to null
   // if(current == tail) break;
}

3 Comments

Op did not say that the double linked list was circular
Thank yout for answering my problem I followed your code and its giving me a error message saying 'else if(!current.next) {' error: bad operand type Node for unary operator '!' Currently trying to fix
Minor typo on my part. Can you try again with revise code above?
0

I see a potential infinite loop with no side effect here. If your list contain a node with node.name set to null then the invocation of current.name.equals(name) results in a NullPointerException. If you are at either end of the list the next or previous pointers will be null which will also result in the same exception. This exception is caught and discarded. Note that this prevents the advance of the current pointer which causes the same iteration to occur. At the very least make sure to print out the exception even if you're not taking any other action. It'll help with debugging.

Your while loop condition is overly complicated. while(current != null) should suffice given that:

Using if(current.name.equals(name)) removes the need for current.name != name. Also, don't use == or != for string comparison. It is a pointer comparison. Most equals methods take care of pointer comparisons.

Use a break or return for flow control here and remove checker boolean. The tail.next should always point to null to signify the end of the list. The only reason I see to have the checker boolean is if delete should remove all matching nodes and you want to know if it happened at least once. From what I see in the code that is not the case.

I would rewrite this as:

public void delete(String name){
    Node current = head;
    while(current != null){
        try{
            if(current.name.equals(name)){
                ...
                return;
                // Do not advance current here. Refer to finally block below.
            }
        } catch(NullPointerException e){
            e.printStackTrace();
            return; // If function should stop on error.
        } finally {current = current.next;} // This prevents the repeat as it always happens.
    }
    System.out.println("Item not found.");
}

Note if you use "break" instead of "return" then the "Item not found." line will always print. You'd have to guard it with an if statement and a flag.

Comments

0
public void delete(String name){
      Node current = head;
      while(current != null){
        if(current.name.equals(name)){
            if(current.prev != null){
             current.prev.next = current.next
            }
            if(current.next != null){
                current.next.prev = current.prev
            }
            System.out.println("Removed node")
            break;
        }
        current = current.next;
      }
}

You could use this logic to delete the node that matches the name(given name is always present) if node is non null.

Comments

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.