Problems with circular singly linked list iterator

Question

I am trying to make an iterator for my circular singly linked list, but I don't undertand how to implement the next() and hasNext() methods. I suspect that I need to either 1) have additional fields in the linked list class or the iterator class, or 2) have references to something else instead of 'head' and 'tail'? My code is below:

public class CircularSinglyLinkedList2<E> implements Iterable<E> {

private Node head;
private Node tail;

public CircularSinglyLinkedList2() {
    head = null;
    tail = null;
}

class Node {
    E data;
    Node next;

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

private boolean isEmpty() {
    return head == null;
}

private int size() {
    int count = 0;
    if(isEmpty()) {
        return 0;
    }
    Node p = head;
    count++;
    p = p.next;
    while(p != head) {
        count++;
        p = p.next;
    }
    return count;
}

public void insert(E data) {
    Node node = new Node(data);
    if(isEmpty()) {
        node.next = node;
        head = node;
        tail = node;
    } else {
        node.next = head;
        head = node;
        tail.next = head;
    }
}

public void delete(E data) {
    if(isEmpty()) {
        return;
    }
    if(head == tail) {
        if(head.data == data) {
            head = null;
            tail = null;
        }
        return;
    }
    Node p = head.next, q = head;
    while(p != head) {
        if(p.data == data) {
            q.next = p.next;
            return;
        }
        q = p;
        p = p.next;
    }
}

public Node search(E data) {
    if(isEmpty()) {
        return null;
    }
    Node p = head;
    if(p.data == data) {
        return p;
    }
    p = p.next;
    while(p != head) {
        if(p.data == data) {
            return p;
        }
        p = p.next;
    }
    return null;
}

public boolean contains(E data) {
    return search(data) != null;
}

public Iterator<E> iterator() {
    return new SLLIterator();
}


private class SLLIterator implements Iterator<E> {

    private Node p;
    private Node q;

    public SLLIterator() {
        if(!isEmpty()) {
            p = head.next;
            q = head;
        }
    }

    @Override
    public boolean hasNext() { doesnt't work
        if(p == q || p == head) {
            return false;
        }
        return true;
    }

    @Override
    public E next() { //doesn't work
        E data = q.data;
        q = p;
        p = p.next;
        return data;
    }

} 

Answer 1

Yes, there is a problem in your iterator. It ignores the last item (the head). For example, imagine that you have just one element in your list. So both head and tail point to it, as well as head.next.

Now, if we ask hasNext() on a fresh iterator on such a list, we are supposed to get true one time, and then, after we call next(), to get false.

But your condition is:

   if(p == q || p == head) {
        return false;
    }

Which means that for a single-element list, you're going to return false, no matter what.

Also, even when we assume we have more than one element, say:

2->1

Then You initialize the iterator with p = head.next and q = head. Your hasNext() will return true, but what does your next() do?

public E next() { //doesn't work
    E data = q.data;
    q = p;
    p = p.next;
    return data;
}

So, it returns the 2 at the beginning, and moves the p and q. That's OK, 2 is printed. But now q points at the 1 and p points again at the 2. And again, your hasNext() sees that p == head and says there are no more elements!

So the element currently pointed to by q, the actual next element, is ignored.

What you should do is have some sort of a flag that tells you when you hit the head the second time as opposed to the first. And there is really no need for two pointers. Here is my version of the iterator:

private class SLLIterator implements Iterator<E> {

    private Node p;
    private boolean atStart;

    public SLLIterator() {
        if(!isEmpty()) {
            p = head;
            atStart = true;
        }
    }

    @Override
    public boolean hasNext() { 
        if(isEmpty() || p == head && ! atStart) {
            return false;
        }
        return true;
    }

    @Override
    public E next() {
        E data = p.data;
        atStart = false;
        p = p.next;
        return data;
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException();
    }

}

This iterator has an atStart flag which starts out as true. So when hasNext() is called on a single-item list, it sees that p points at the head, but it also sees that this is the first time it does, and so it returns true.

And the next() method, in this case, will give you the data pointed at by p, which is the data in the head. At this point, atStart is changed to false, so although p is advanced and circles back, and is again head, the next time we call hasNext(), we won't return true again.

And if you have a longer list like 2->1, you'll get the 2 the first time, and p is advanced to the 1 and atStart is false.

Now the second time, hasNext() sees that p is pointing at the 1 which is not the head so it returns true. And in the next() we return the 1, and advance p to the head.

Calling hasNext() again will stop because p is back to the head, but atStart is false.

Answer 2

Your hasNext() is not completely correct. Because of your p == head condition, you will always miss out printing the last element. So checking p will not help in deciding hasNext().

Ideally what you want to check is q == head, this is when you finish iterating the all the elements and came back to the starting element and your hasNext() should return false, but if you simply check q == head, it's not going to work because for your starting element itself it will fail. So use a flag to find out whether you came back or you are visiting for the first time.

I would suggest you to do something like this:

use visitingAgain flag.

boolean visitingAgain = false;

Use it in your hasNext() like the following.

@Override
public boolean hasNext() { 
   if (p == q || (q == head && visitingAgain)){
      return false;
   }
   visitingAgain = true; // once you start iterating change this flag.
   return true;
}

NOTE I did not check your insert and delete logic completely, if this does not work just make sure that your other functions are correct. Let me know if there is any problem.