Can someone review my doubly linked list?

Question

Here is the implementation with the interface below:

public class DoublyLinkedList<E> implements ListInterface<E>, ListIteratorInterface<E> {
    private DoublyLinkedListNode<E> head;
    private DoublyLinkedListNode<E> tail;
    private DoublyLinkedListNode<E> currentNode;
    private int size;

    /**
     * Create a new empty DoublyLinkedList object.
     */
    public DoublyLinkedList() {
    this.currentNode = this.head = new DoublyLinkedListNode<E>(null, null,
        null);
    this.tail = new DoublyLinkedListNode<E>(null, this.head, null);
    this.head.setNextNode(this.tail);
    this.size = 0;
    }

    @Override
    public void insert(E item) {
    this.currentNode.setNextNode(new DoublyLinkedListNode<E>(item,
        this.currentNode, this.currentNode.getNextNode()));
    this.currentNode.getNextNode().getNextNode()
        .setPreviousNode(this.currentNode.getNextNode());
    this.size++;
    }

    @Override
    public void append(E item) {
    this.tail.setPreviousNode(new DoublyLinkedListNode<E>(item, this.tail
        .getPreviousNode(), this.tail));
    this.tail.getPreviousNode().getPreviousNode()
        .setNextNode(this.tail.getPreviousNode());
    this.size++;
    }

    @Override
    public E remove() {
    if (this.currentNode.getNextNode() == this.tail) {
        return null; // empty linked list
    }
    E item = this.currentNode.getNextNode().getValue(); // remember value to
                                // be deleted
    this.currentNode.getNextNode().getNextNode()
        .setPreviousNode(this.currentNode);

    // remove current node
    this.currentNode.setNextNode(this.currentNode.getNextNode()
        .getNextNode());
    this.size--;
    return item;
    }

    @Override
    public void clear() {
    // drop access to all other nodes
    this.head.setNextNode(null);
    this.currentNode = this.head = new DoublyLinkedListNode<E>(null, null,
        null);
    this.tail = new DoublyLinkedListNode<E>(null, this.head, null);
    this.head.setNextNode(this.tail);
    this.size = 0;
    }

    @Override
    public void moveToStart() {
    this.currentNode = this.head;
    }

    @Override
    public void moveToEnd() {
    this.currentNode = this.tail.getPreviousNode();
    }

    @Override
    public boolean previous() {
    if (this.currentNode != this.head) {
        this.currentNode = this.currentNode.getPreviousNode();
        return true;
    } else {
        return false; // no node before head node
    }
    }

    @Override
    public boolean next() {
    if (this.currentNode != this.tail.getPreviousNode()) {
        this.currentNode = this.currentNode.getNextNode();
        return true;
    } else {
        return false;
    }
    }

    @Override
    public int length() {
    return this.size;
    }

    @Override
    public int currentPosition() {
    DoublyLinkedListNode<E> tempNode = this.head;
    int indexOfCurrentNode;
    for (indexOfCurrentNode = 0; this.currentNode != tempNode; indexOfCurrentNode++) {
        tempNode = tempNode.getNextNode();
    }
    return indexOfCurrentNode;
    }

    @Override
    public void moveCurrentToPosition(int position) {
    if (position < 0 || position > this.size) {
        throw new IllegalArgumentException(
            "In method moveCurrentToPosition of class "
                + "DoublyLinkedList the input node postion to be "
                + "removed is out of bounds");
    }
    this.currentNode = this.head;
    for (int i = 0; i < position; i++) {
        this.currentNode = this.currentNode.getNextNode();
    }
    }

    @Override
    public E getValue() {
    if (this.currentNode.getNextNode() == this.tail) {
        return null;
    } else {
        return this.currentNode.getNextNode().getValue();
    }
    }

    /**
     * Creates a easy to read String representation of the doubly linked lists
     * contents.
     *
     * Example 1: < 1 2 3 4 | 5 6 >
     *
     * The vertical bar = the link immediately after the current node.
     *
     * @author Clifford A. Shaffer
     */
    public String toString() {
    int oldPosition = this.currentPosition();
    int length = this.length();
    StringBuffer linkedListAsString = new StringBuffer((length() + 1) * 4);

    this.moveToStart();
    linkedListAsString.append("< ");
    for (int i = 0; i < oldPosition; i++) {
        linkedListAsString.append(this.getValue());
        linkedListAsString.append(" ");
        this.next();
    }
    linkedListAsString.append("| ");
    for (int i = oldPosition; i < length; i++) {
        linkedListAsString.append(this.getValue());
        linkedListAsString.append(" ");
        this.next();
    }
    linkedListAsString.append(">");
    this.moveCurrentToPosition(oldPosition);

    return linkedListAsString.toString();
    }
}

Here is the ListInterface and ListIteratorIterface

public interface ListInterface<E> {
    /**
     * Insert an element behind the current position. Must check that the linked
     * list's capacity is not exceeded.
     *
     * @param item
     *            Item to be inserted.
     */
    public void insert(E item);

    /**
     * Insert an element after the last element in the list.
     *
     * @param item
     *            Item to be appended.
     */
    public void append(E item);

    /**
     * Remove all contents from the list.
     */
    public void clear();



    /**
     * @return The number of items in the list.
     */
    public int length();

    /**
     * @param position
     *            Position to move current to.
     */
    public void moveCurrentToPosition(int position);
}


public interface ListIteratorInterface<E> {
    /**
     * Remove the element after the current element and return the value of the
     * removed element.
     *
     * @return The element that was removed.
     */
    public E remove();

    /**
     * Move current position to first element.
     */
    public void moveToStart();

    /**
     * Move current position to last element.
     */
    public void moveToEnd();

    /**
     * Move the current position one element before. No change if already at the
     * beginning.
     *
     * @return True if moved to previous position; otherwise return false.
     */
    public boolean previous();

    /**
     * Move the current position one element after. No change if already at the
     * end.
     *
     * @return True if moved to current position; otherwise return false.
     */
    public boolean next();

    /**
     * @return The current position.
     */
    public int currentPosition();

    /**
     * @return The current item in the current position.
     */
    public E getValue();
}

Answer 1

1) I would rename getValue() in your iterator to currentValue() to make it consistent with currentPosition() and also to express in name what the comment says (get current item).

2) I don't like how the ListInterface and ListIteratorInterface implementations introduce a state in the list for the current position. It intermingles data representation with iteration. Also both ListInterface and ListIteratorInterface contain methods regarding a current position so they are no longer clearly separated.

Have a look at C++ iterators. They remove the iteration concern completely from the list (by keeping the iteration state in the iterator and not in the list) yet allow you to do what you want: insert an element at a specific position (Insert(iterator, value)).

• You should remove the currentNode concept from your implementation and make an actual iterator object keeping the state.
• Remove the moveCurrentPosition from the ListInterface - that interface should have nothing to do with iteration except maybe accept an iterator for insert so you can insert an element at a specific position.