HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Double Linked List remove method

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
methodremovedoublelistlinked

Problem

I am implementing some basic data structures to freshen up my memory and I wanted my DoubleLinkedList remove method to be reviewed. I feel like it has extra checks but I could not think of another way to implement.

My implementation holds both the head and the tail of the list making things a bit more complex.

remove method:

public void remove(T data) {
    if (isEmpty()) {
        throw new NoSuchElementException();
    }

    for (Node current = this.head; current != null; current = current.getNext()) {
        if (current.getData().equals(data)) {
            Node previous = current.getPrevious();
            Node next = current.getNext();

            if (this.size == 1) { // At head with size == 1
                this.head = this.tail = null;
            } else if (previous == null) { // At head with size > 1
                this.head = next;
                next.setPrevious(previous);
            } else if (next == null) { // At tail
                previous.setNext(next);
                this.tail = previous;
            } else { // Rest of cases
                previous.setNext(next);
                next.setPrevious(previous);
            }
            this.size--;
            return;
        }
    }
    throw new NoSuchElementException();
}

Solution

I have couple suggestions:

Separate Logic

I would separate lookup and modify logic. Add a function to remove element and call it from this one leaving in this one only logic to lookup an item.

public void remove(Node data) {
    if (!data) {
        throw new NoSuchElementException();
    }

    // Removing logic here
    ...
}

public void remove(T data) {
    return remove(search(data));
}

public Node search(T data) {
    for (Node current = this.head; current != null; current = current.getNext()) {
        if (current.getData().equals(data)) {
            return current;
        }
    }

    return null; 
}


Simplify Checks

I think this should work and cover all your cases, no?

// If this is head of the list
if (previous == null) { 
    this.head = next;
}
else {
    previous.setNext(next);
}

// If this is tail of the list
if (next == null) { 
    this.tail = previous;
}
else {
    next.setPrevious(previous);
}

Code Snippets

public void remove(Node<T> data) {
    if (!data) {
        throw new NoSuchElementException();
    }

    // Removing logic here
    ...
}

public void remove(T data) {
    return remove(search(data));
}

public Node<T> search(T data) {
    for (Node<T> current = this.head; current != null; current = current.getNext()) {
        if (current.getData().equals(data)) {
            return current;
        }
    }

    return null; 
}
// If this is head of the list
if (previous == null) { 
    this.head = next;
}
else {
    previous.setNext(next);
}

// If this is tail of the list
if (next == null) { 
    this.tail = previous;
}
else {
    next.setPrevious(previous);
}

Context

StackExchange Code Review Q#66413, answer score: 5

Revisions (0)

No revisions yet.