patternjavaModerate
Filtered Iterator
Viewed 0 times
iteratorfilteredstackoverflow
Problem
I have written a wrapper for Java's
When the filter is null, all items should be iterated.
Is there anything I can improve on here? I'm not all that fond of
Iterator class using Java 7, which is designed to only iterate items that match a certain filter.When the filter is null, all items should be iterated.
Filter.javapublic interface Filter {
boolean apply(T type);
}FilteredIterator.javaimport java.util.Iterator;
public class FilteredIterator implements Iterator {
private Iterator iterator;
private Filter filter;
private E iteratorNext;
private boolean iteratorHasNext;
public FilteredIterator(Iterator iterator, Filter filter) {
this.iterator = iterator;
this.filter = filter;
findNextValid();
}
private void findNextValid() {
iteratorHasNext = iterator.hasNext();
while (iterator.hasNext()) {
iteratorNext = iterator.next();
if (filter == null || filter.apply(iteratorNext)) {
iteratorHasNext = true;
break;
}
}
}
@Override
public boolean hasNext() {
return iteratorHasNext;
}
@Override
public E next() {
E nextValue = iteratorNext;
findNextValid();
return nextValue;
}
@Override
public void remove() {
iterator.remove();
}
}Is there anything I can improve on here? I'm not all that fond of
findNextValid's implementation but I don't know what I can do to improve it.Solution
Correctness
You can't support
Design
You twist the code to make a null filter equal to a filter that accepts everything. Don't do that. Write a filter that accepts everything and use that instead.
Your class should be final. It's not designed to be extended.
You really need to document the
EDIT: I'm not a big fan of using
EDIT: You can/should also move the Accept All filter to its own top-level class and make it public. You need it anyway .. may as well let clients specify it directly.
Implementation
You don't need
With all these changes, your code might look more like:
You can't support
remove() because your iterator must always be at least one step behind the iterator being wrapped.Design
You twist the code to make a null filter equal to a filter that accepts everything. Don't do that. Write a filter that accepts everything and use that instead.
Your class should be final. It's not designed to be extended.
You really need to document the
Filter#apply(). That's not an intuitive method name. I'd rather see something like #accept(). When I apply a filter, I expect to pass in a collection and get back a filtered collection. You're asking the filter if a specific element is acceptable. That naming would also be more consistent with the JDK classes named XXXFilter, which use the accept name.EDIT: I'm not a big fan of using
null to indicate a default behavior. If they don't have to specify a filter, give them another constructor that doesn't ask for one. It can chain to the two-arg filter with a new Accept All filter. If they call a constructor asking for a filter and don't give one, I'd throw an NPE back at them.EDIT: You can/should also move the Accept All filter to its own top-level class and make it public. You need it anyway .. may as well let clients specify it directly.
Implementation
You don't need
iteratorHasNext at all. Just keep the next value. If there is none, set it to null. Your hasNext() implementation can check for null and return correctly.With all these changes, your code might look more like:
public final class FilteredIterator implements Iterator {
private final Iterator iterator;
private final Filter filter;
private boolean hasNext = true;
private E next;
public FilteredIterator(final Iterator iterator, final Filter filter) {
this.iterator = iterator;
Objects.requireNonNull(iterator);
if (filter == null) {
this.filter = new AcceptAllFilter();
} else {
this.filter = filter;
}
this.findNext();
}
@Override
public boolean hasNext() {
return this.next != null;
}
@Override
public E next() {
E returnValue = this.next;
this.findNext();
return returnValue;
}
@Override
public void remove() {
throw new UnsupportedOperationException();
}
private void findNext() {
while (this.iterator.hasNext()) {
this.next = iterator.next();
if (this.filter.accept(this.next)) {
return;
}
}
this.next = null;
this.hasNext = false;
}
private static final class AcceptAllFilter implements Filter {
public boolean accept(final T type) {
return true;
}
}
}Code Snippets
public final class FilteredIterator<E> implements Iterator<E> {
private final Iterator<E> iterator;
private final Filter<E> filter;
private boolean hasNext = true;
private E next;
public FilteredIterator(final Iterator<E> iterator, final Filter<E> filter) {
this.iterator = iterator;
Objects.requireNonNull(iterator);
if (filter == null) {
this.filter = new AcceptAllFilter<E>();
} else {
this.filter = filter;
}
this.findNext();
}
@Override
public boolean hasNext() {
return this.next != null;
}
@Override
public E next() {
E returnValue = this.next;
this.findNext();
return returnValue;
}
@Override
public void remove() {
throw new UnsupportedOperationException();
}
private void findNext() {
while (this.iterator.hasNext()) {
this.next = iterator.next();
if (this.filter.accept(this.next)) {
return;
}
}
this.next = null;
this.hasNext = false;
}
private static final class AcceptAllFilter<T> implements Filter<T> {
public boolean accept(final T type) {
return true;
}
}
}Context
StackExchange Code Review Q#112109, answer score: 10
Revisions (0)
No revisions yet.