Iterator to remove duplicate element

Question

The code implements custom iterator, which skips duplicate integers. The array expected is to be sorted.

public class GenericIterator<K> implements Iterator<K> {
 private K[] array; // Array will be set here
 private int index;
 GenericIterator(K[] array) {
 this.array = array;
 index = 0;
 }
 /*
 * 
 * 
 * (non-Javadoc)
 * 
 * @see java.util.Iterator#hasNext() /** Returns <tt>true</tt> if this 
 * iterator has more elements when traversing the array in the forward
 * direction. (In other words, returns <tt>true</tt> if <tt>next</tt> would
 * return an element rather than throwing an exception.)
 * 
 * @return <tt>true</tt> if the array iterator has more elements when
 * traversing the array in the forward direction.
 */
 @Override
 public boolean hasNext() {
 return !(array.length == index);
 }
 /*
 * 
 * 
 * (non-Javadoc)
 * 
 * @see java.util.Iterator#hasNext()
 * 
 * /** Returns the next element in the array. This method may be called
 * repeatedly to iterate through the array, or intermixed with calls to
 * <tt>previous</tt> to go back and forth. (Note that alternating calls to
 * <tt>next</tt> and <tt>previous</tt> will return the same element
 * repeatedly.)
 * 
 * @return the next element in the array.
 * 
 */
 @Override
 public K next() {
 K outputElement = array[index++];
 if (index == 0) {
 index++;
 }
 while (hasNext()) {
 if (array[index] == array[index - 1]) {
 index++;
 } else {
 break;
 }
 }
 return outputElement;
 }
}

• chillworld: Can you specify what exactly what you wanted to review or just ask for a general review. The last sentence is a little short for that.
• JS1: I don't even understand the purpose of this code. It seems like the array is assumed to be sorted but nowhere in the comments or question does it say this is a prerequisite. Therefore either the code is broken, or the purpose is unclear.
• Mast: To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question
• chillworld: @JS1 I think he want to have an iterator who skip's the next element if that element is the same.
• chillworld: Why using generics and call it GenericIterator if you only expect Integer?

Answer 1

The most bizarre aspect of the code is the comments, where the JavaDoc starts in the middle of a non-JavaDoc comment. Why not write standard JavaDoc?

The JavaDoc for next() refers to a previous method — but no such method exists.

By convention, type variables are named T instead of K.

The body of hasNext() would be slightly better written as return array.length != index;.

The contract for next() is that it should throw a NoSuchElement Exception if there are no more results, not an ArrayIndexOutOfBoundsException.

Are you sure that you want to test for equality using == instead of .equals()? A typical use case involving Integer[] or String[] wouldn't behave the way I would expect.

The if (index == 0) check can never succeed (unless index wraps around due to integer overflow!), so it's dead code. The flow of control for the while loop is slightly cumbersome, in my opinion. I would personally prefer a for loop to handle all of the index advancement and end-of-array testing.

public T next() throws NoSuchElementException {
 if (index >= array.length) {
 throw new NoSuchElementException();
 }
 T output = array[index];
 for (index++; index < array.length; index++) {
 if (array[index] == null) {
 if (array[index - 1] != null) {
 break;
 }
 } else if (!array[index].equals(array[index - 1])) {
 break;
 }
 }
 return output;
}

Answer 2

Not much to review, but, if I read the Java documentation, you don't put the complete class in it.

Because you didn't put the full class I come to the next thing :

K outputElement = array[index++];
if (index == 0) {
 index++;
}

What the hell are you doing?

index is instantiated at 0, the line before is index++ so index is never 0.

---

Second point, you missed some errors.
When you are at the last element and you ask next() you will get an OutOfBoundsException.

Rather, it's better to check if your at the last element and if so, just return null.

The OutOfBoundsException is an unchecked exception, so nobody who will use your class will expect and handle this error, and then the problems are coming.

---

Also add JavaDoc what the Iterator returns when you are at the last element.

---

Then another point; the constructor has this line :

index = 0;

It's obsolete. An int default value is always 0.
Because it's not a static variable, it's instantiated when you create the class; so it's 0.

---

Generics:

With the new description in your question, a few more things come up.
I don't know if you know how to use generics or it's just a fault but:

• You expect Integer so I should rename the class name to UniqueIntegerIterator or something like that.
• Because public class GenericIterator<K> implements Iterator<K> means that you can insert any type of Object it has a lot more usage then Integer only.
  If you want to make it only for Integer, the declaration should change to public class GenericIterator implements Iterator<Integer>.
  Of course the K[] should be changed to Integer[]

Expect bad usage of your class.

You state that you expect a sorted integer array in the constructor. You may ask:

What if this isn't the case?

It's really important to ask yourself this question.
Normally, when you have some requirements, you check for that and throw an AssertionError when the requirements are not met.
In this case and in my own perception, I should sort the array in the constructor so I know the requirement is done.
Also, when I do this I rename the class again to UniqueSortedIntegerIterator so the people who are going to use this class understand they get unique values from a sorted collection, in this case an array. The Object behind is Integer and it acts like an Iterator.