Iterator Returns Itself
ID |
java.iterator_return |
Severity |
high |
Remediation Complexity |
medium |
Remediation Risk |
low |
Remediation Effort |
medium |
Resource |
Code Smell |
Language |
Java |
Tags |
best-practice, reliability |
Description
Reports a class that implements both Iterable and Iterator and whose iterator() method
returns this. Iterator carries the state of one iteration — the current position — while
Iterable is the factory that is supposed to hand out a fresh Iterator on every call. Returning
this collapses the two: there is only ever one cursor, shared by every iteration over the object.
Rationale
class Ring implements Iterable<String>, Iterator<String> {
private String[] items;
private int idx = 0;
public boolean hasNext() {
return idx < items.length;
}
public String next() {
return items[idx++];
}
public Iterator<String> iterator() {
return this; // FLAW — every call hands out the same cursor
}
}Two separate for loops over the same Ring now interfere with each other: the second loop
starts wherever the first one left off, or sees nothing at all if the first already ran to
completion. Nested iteration over the same instance is worse — both loops advance the one shared
cursor, so neither sees a consistent sequence.
Remediation
Keep the iteration state in a separate object and return a new one from iterator(), either as
a dedicated class or as an anonymous Iterator.
class Ring implements Iterable<String> {
private String[] items;
public Iterator<String> iterator() {
return new Iterator<String>() { // OK — a fresh cursor for every call
private int idx = 0;
public boolean hasNext() {
return idx < items.length;
}
public String next() {
return items[idx++];
}
};
}
}