CODEWITHSUNDEEP
javaoopdoubly-linked-list
Grwolfy
Grwolfy

Reputation: 19

Java Doubly Linked list delete method

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)

Upvotes: 1

Views: 636

Answers (4)

user9706
user9706

Reputation:

"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;
}

Upvotes: 1

Vipin Sharma
Vipin Sharma

Reputation: 107

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.

Upvotes: 0

MYousefi
MYousefi

Reputation: 1008

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.

Upvotes: 0

Thiyagu
Thiyagu

Reputation: 17890

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.

Upvotes: 1

Related Questions