Self-made linked list iterator I recently made a linked list for myself, the code of which was posted here, if anyone would be interested. Now, as the next task, I had to make an Iterator for my list, which is working properly.

public Iterator&lt;T&gt; iterator() {
    final MyLinkedList&lt;T&gt; list = this;
    return new Iterator&lt;T&gt;() {
        final Node&lt;T&gt; firstNode = list.firstNode;
        Node&lt;T&gt; currentNode = null;
        @Override
        public boolean hasNext() {
            if (list.isEmpty()) {
                return false;
            } else if (currentNode == null){
                return true;
            } else if (currentNode == list.lastNode){
                return false;
            }
            return true;
        }
        @Override
        public T next() {
            if (list.isEmpty()){
                throw new NoSuchElementException();
            } else if (currentNode == null){
                this.currentNode = firstNode;
                return currentNode.data;
            } else if (currentNode.nextNode == null) {
                throw new NoSuchElementException();
            }
            this.currentNode = currentNode.nextNode;
            return currentNode.data;
        }
    };
}


Here are the variables of the LinkedList class:

private Node&lt;T&gt; lastNode;
private Node&lt;T&gt; firstNode;
private int size;


This is a Node&lt;U&gt;:

private static class Node&lt;U&gt;{
    private Node&lt;U&gt; nextNode = null;
    private Node&lt;U&gt; previousNode = null;
    private int index;
    private U data;

    private Node(U data){
        this.data = data;
    }
}


And this is how I add a new Node:

public boolean add(T data) {
    if (data == null) {
        return false;
    }

    Node&lt;T&gt; currentNode = new Node&lt;T&gt;(data);

    if (this.isEmpty()){
        currentNode.previousNode = null;
        currentNode.index = 0;
        this.firstNode = currentNode;
    } else {
        currentNode.previousNode = lastNode;
        lastNode.nextNode = currentNode;
        currentNode.index = lastNode.index + 1;
    }
    this.lastNode = currentNode;
    this.size++;
    return true;
}


Could this be made significantly faster? Does the code comply with the Java coding conventions?
Does the code comply with the Java coding conventions?


I think it does comply quite well. A tiny thing is that it would be better to put a space between ){, for example in if (list.isEmpty()){. More importantly, it can be simplified.



There is no need for the inner list variable pointing to this.
The inner (non-static) class has direct access to the containing class' fields and methods: it can access firstNode directly from the containing list. So in this code:


final MyLinkedList&lt;T&gt; list = this;
return new Iterator&lt;T&gt;() {
    final Node&lt;T&gt; firstNode = list.firstNode;



You can drop both the list variable and the firstNode variable.



It's a minor thing, but currentNode can be private.



The hasNext method can be simplified by a lot:


@Override
public boolean hasNext() {
    if (list.isEmpty()) {
        return false;
    } else if (currentNode == null){
        return true;
    } else if (currentNode == list.lastNode){
        return false;
    }
    return true;
}



This is equivalent:

@Override
public boolean hasNext() {
    return !isEmpty() &amp;&amp; currentNode != lastNode;
}





  Could this be made significantly faster?


Slightly. Notice that the isEmpty() check is pointless to perform for every iteration of a non-empty list. So as a minor optimization,
you could check for emptiness once, in the beginning,
and then omit all the isEmpty() checks from the rest of the code.

Suggested implementation

Putting it all together:

public Iterator&lt;T&gt; iterator() {
    if (isEmpty()) {
        return Collections.&lt;T&gt;emptyList().iterator();
    }
    return new Iterator&lt;T&gt;() {
        private Node&lt;T&gt; currentNode = null;

        @Override
        public boolean hasNext() {
            return currentNode != lastNode;
        }

        @Override
        public T next() {
            if (currentNode == null) {
                currentNode = firstNode;
                return currentNode.data;
            }
            if (currentNode.nextNode == null) {
                throw new NoSuchElementException();
            }
            currentNode = currentNode.nextNode;
            return currentNode.data;
        }
    };
}


Unit testing

I didn't just refactor aggressively. I wrote unit tests first to know that I'm not breaking anything. After that I could go ahead and refactor aggressively:

@Test
public void testEmpty() {
    MyLinkedList&lt;Integer&gt; list = new MyLinkedList&lt;&gt;();
    assertFalse(list.iterator().hasNext());
}

@Test(expected = NoSuchElementException.class)
public void throwIfNextOnEmpty() {
    MyLinkedList&lt;Integer&gt; list = new MyLinkedList&lt;&gt;();
    list.iterator().next();
}

@Test(expected = NoSuchElementException.class)
public void throwIfIterateBeyond() {
    MyLinkedList&lt;Integer&gt; list = new MyLinkedList&lt;&gt;();
    list.add(1);
    list.add(2);
    Iterator&lt;Integer&gt; iter = list.iterator();
    iter.next();
    iter.next();
    iter.next();
}

@Test
public void testStandardIteration() {
    MyLinkedList&lt;Integer&gt; list = new MyLinkedList&lt;&gt;();
    Integer[] items = { 12, 3, 4 };
    for (int i : items) {
        list.add(i);
    }
    Iterator&lt;Integer&gt; iter = list.iterator();
    for (Integer item : items) {
        assertTrue(iter.hasNext());
        assertEquals(item, iter.next());
    }
    assertFalse(iter.hasNext());
}


These may not be perfect, and might not cover all corner cases, but I hope they are enough to get you started.