Linked List Delete Node Function

Question

This code is supposed to delete every node from a linked list with a given value.

Are there any logical errors with this implementation? Did I do anything wrong or is there an area of this implementation that needs significant improvement. Any suggestions?

public boolean delete(int d) {
 Node tmpNode = head;
 Node prevNode = null;
 boolean deletedANode = false;
 if (head == null) {
 return deletedANode;
 }
 while (tmpNode != null) {
 if (tmpNode.data == d) {
 if (tmpNode == head) {
 head = head.next;
 }
 else {
 prevNode.next = tmpNode.next;
 }
 deletedANode = true;
 }
 prevNode = tmpNode;
 tmpNode = tmpNode.next;
 }
 return deletedANode;
}

• rolfl: You need to give some more detail on what the delete is supposed to do... like, at the moment it keeps searching even after it's deleted a node... is that intentional?
• Adam Johns: yes, it is supposed to delete every node with the value
• 200_success: I suggest renaming the method to deleteAll(value).

Answer 1

Your code is buggy, if you have two Nodes with the same value in succession it will corrupt the list....

prevNode will be set to the deleted node tempNode, and if the next value also matches the input value you will be working with the wrong node as prevNode.

Also, why is d a good name for the input search value?

To avoid future bugs it is convenient to mark constant input values as final. It also can potentially help with performance

You need to change some code:

// added final, change input to `searchValue`
public boolean delete(final int searchValue) {
 Node tmpNode = head;
 Node prevNode = null;
 boolean deletedANode = false;
 /*
 This code is redundant, the while-loop does the same effective thing.
 if (head == null) {
 return deletedANode;
 }
 */
 while (tmpNode != null) {
 if (tmpNode.data == searchValue) {
 if (tmpNode == head) {
 head = head.next;
 } else { // fixed indenting/newline
 prevNode.next = tmpNode.next;
 }
 // fixed indenting
 deletedANode = true;
 } else {
 // only advance the prevNode when there's no match.
 prevNode = tmpNode;
 }
 tmpNode = tmpNode.next;
 }
 return deletedANode;
}

• Adam Johns: Good catch with bug of wrongly advancing tmpNode. Thanks for the help.