Java: NoSuchElementException when iterating through ArrayList

Question

I want to delete duplicate elements and therefore iterate through a ArrayList and compare two consecutive elements. (Persons are comparable)

ArrayList<Person> persons = getHelper().findAllPersons();
Collections.sort(persons);
ListIterator<Person> it = persons.listIterator();
if(it.hasNext()) {
    Person tmp = it.next();
    while(it.hasNext()) {
        if(tmp.getLastDiscovered() == it.next().getLastDiscovered()) {
            getHelper().delete(tmp);
        }
    tmp = it.next();
    }
}

I get a NoSuchElementException at tmp = it.next();

Shouldn't the while(it.hasNext()) prevent that?

Answer 1

The problem is you are calling it.next() twice, which will advance the iterator two times.

You should store the value to avoid repeating the side-effect.

    Person person = it.next();
    if (tmp.getLastDiscovered() == person.getLastDiscovered()) {
        getHelper().delete(tmp);
    }
    tmp = person;

Alternatively, you could use the for-each loop to avoid needing to interact with the iterators (I assume all Person are not null):

Person tmp = null;
for (Person person : persons) {
    if (tmp != null && tmp.getLastDiscovered() == person.getLastDiscovered()) {
        getHelper().delete(tmp);
    }
    tmp = person;
}

Answer 2

You're calling it.next() twice (potentially) for each it.hasNext() call, hence your error.

If you want to remove duplicates, why not just populate a TreeSet (providing the appropriate Comparator) with your list ? The semantics of a Set are such that you'll have a distinct set of elements.

Answer 3

If you're using JDK 1.5.0 or later (which you most likely are, since it was released in 2004), you can use a foreach loop to obviate the iterator altogether, greatly simplifying the code.

ArrayList<Person> persons = getHelper().findAllPersons();
Collections.sort(persons);
for (Person person : persons) {
    if(tmp.getLastDiscovered() == person.getLastDiscovered()) {
        getHelper().delete(tmp);
    }
}

Answer 4

while(it.hasNext()) {
        if(tmp.getLastDiscovered() == it.next().getLastDiscovered()) {
            getHelper().delete(tmp);
        }

After that 'while' you are coming to end of the list. Then when it hasn't got next value, you are calling the line below.

tmp = it.next();

That gives you an exception.