Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
    
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? –  rolfl Dec 11 '13 at 3:20
    
yes, it is supposed to delete every node with the value –  Adam Johns Dec 11 '13 at 3:21
    
I suggest renaming the method to deleteAll(value). –  200_success Dec 11 '13 at 19:50

1 Answer 1

up vote 10 down vote accepted

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;
}
share|improve this answer
    
Good catch with bug of wrongly advancing tmpNode. Thanks for the help. –  Adam Johns Dec 15 '13 at 5:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.