HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Filtered Iterator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
iteratorfilteredstackoverflow

Problem

I have written a wrapper for Java's 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.java

public interface Filter {
    boolean apply(T type);
}


FilteredIterator.java

import 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 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.