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.