Custom written Iterator for a Set throws Exception in for-each loop

Question

I'm trying to write a custom made Iterator for a Set that I made. I'm a bit confused about the contract for the Interface Iterable. It has three methods: next(), hasNext() and remove(). My set is immutable so I'm planning on throwing an UnsupportedOperationException for the method remove(). It is also so called "lazily generated" ie elements are not stored in a memory but rather created when needed, but that's neither here nor there.

The javadoc for the Iterator's next() method is as follows:

E next()

Returns the next element in the iteration.
Returns:
the next element in the iteration
Throws:
NoSuchElementException - if the iteration has no more elements

and for hasNext() is:

boolean hasNext()

Returns true if the iteration has more elements. (In other words, returns 
true if next() would return an element rather than throwing an exception.)

Going by these rules, I started implementing my Set and Iterator, getting this:

import java.util.AbstractSet;
import java.util.Iterator;

public class PrimesBelow extends AbstractSet<Integer>{

    int max;
    int size;

    public PrimesBelow(int max) {
        this.max = max;
    }

    @Override
    public Iterator<Integer> iterator() {
        return new SetIterator<Integer>(this);
    }

    @Override
    public int size() {
        if(this.size == -1){
            System.out.println("Calculating size");
            size = calculateSize();
        }else{
            System.out.println("Accessing calculated size");
        }
        return size;
    }

    private int calculateSize() {
        int c = 0;
        for(Integer p: this)
            c++;
        return c;
    }

    public static void main(String[] args){
        PrimesBelow primesBelow10 = new PrimesBelow(10);
        for(int i: primesBelow10)
            System.out.println(i);
        System.out.println(primesBelow10);
    }
}

.

import java.util.Iterator;
import java.util.NoSuchElementException;

public class SetIterator<T> implements Iterator<Integer> {
    int max;
    int current;
    public SetIterator(PrimesBelow pb) {
        this.max= pb.max;
        current = 1;
    }

    @Override
    public boolean hasNext() {
        if(current < max) return true;
        else return false;
    }

    @Override
    public Integer next() {
        while(hasNext()){
            current++;
            if(isPrime(current)){
                System.out.println("returning "+current);
                return current;
            }
        }
        throw new NoSuchElementException();
    }

    private boolean isPrime(int a) {
        if(a<2) return false;
        for(int i = 2; i < a; i++) if((a%i)==0) return false;
        return true;
    }
}

This seemed fine by my, next() return the next value and throws an Exception when there is no more. hasNext() should return true if there are more values to iterate over and false otherwise. The output from the main loop however is this:

returning 2
2
returning 3
3
returning 5
5
returning 7
7
Exception in thread "main" java.util.NoSuchElementException
    at SetIterator.next(SetIterator.java:27)
    at SetIterator.next(SetIterator.java:1)
    at PrimesBelow.main(PrimesBelow.java:38)

So it seems the Exception isn't handled in the for each loop. How do I write a custom Iterator that I can use so that this works? I tried returning null instead of throwing an Exception but that just gives a NullPointerException.

Should I return null when the Iterator is finished, or throw an Exception? Javadoc says next() should throw an Exception but when I hover over the overriden method next() in Eclipse the signature doesn't show throws NoSuchElementException so I'm very confused as to what the contract says. It seems very weird to me to return null when I'm done since the collection could potentially contain null elements.

Thank for the help.

Why do you loop over hasNext() in your implementation of next()? You should only check if there is a next element without a while-loop. — dpr
I think I did it because that's where the logic is to check if the Iterator can return more elements, to avoid code duplication and writing if(current < max) two different times. current iterates over all natural numbers, so it needs to be checked that current is a prime, and that it is smaller than the max value. Since not all natural numbers are prime, I need to keep increasing current until I hit a prime number or the limit, that's why the while loop. — user1661303
@user1661303 the logic doesn't make sense mate, next() should return a single value (not looping through it). and this code will always throw an exception because after looping through all elements u end up throwing an exception regardless — nafas

dpr dpr · Accepted Answer · 2017-08-18T12:28:00

Suppose the last prime returned by your code is 7. Then current will be 7 which is obviously < 10. So hasNext() will return true. However there is no more prime greater than 7 but smaller than 10, so the next call to next() will produce the NoSuchElementException. Currently your code will only work, if max is a prime.

You will need to validate, that there is an additional prime available in hasNext().

Custom written Iterator for a Set throws Exception in for-each loop

2 Answers