1
\$\begingroup\$

I have this small utility class that allows me to iterate over a series of collections. Please tell me anything that comes to mind.

JointIterable.java

package net.coderodde.util;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import java.util.NoSuchElementException;

public final class JointIterable<E> implements Iterable<E> {

    /**
     * Holds the list of iterable objects.
     */
    private final List<Iterator<E>> iterators;

    /**
     * Constructs a new joint iterable. 
     * 
     * @param iterables the iterable object to iterate.
     */
    public JointIterable(Iterable<E>... iterables) {
        if (iterables == null) {
            this.iterators = new ArrayList<>(0);
        } else {
            this.iterators = new ArrayList<>(iterables.length);

            for (Iterable<E> iterable : iterables) {
                if (iterable != null) {
                    this.iterators.add(iterable.iterator());
                }
            }
        }
    }

    @Override
    public Iterator<E> iterator() {
        return new JointIterator();
    }

    private final class JointIterator implements Iterator<E> {

        private int currentIteratorIndex = 0;

        @Override
        public boolean hasNext() {
            while (currentIteratorIndex < iterators.size() &&
                    !iterators.get(currentIteratorIndex).hasNext()) {
                currentIteratorIndex++;
            }

            return currentIteratorIndex < iterators.size();
        }

        @Override
        public E next() {
            if (!hasNext()) {
                throw new NoSuchElementException("Nothing left for iteration.");
            }

            return iterators.get(currentIteratorIndex).next();
        }
    }
}

JointIterableDemo.java

package net.coderodde.util;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

public final class JointIterableDemo {

    public static void main(String[] args) {
        Collection<String> yeahs1 = Arrays.asList("one", "two");
        Set<String> yeahs2 = new HashSet<>();
        List<String> yeahs3 = new ArrayList<>(Arrays.asList("three", 
                                                           "four",
                                                           "five"));
        Set<String> yeahs4 = new HashSet<>(Arrays.asList("six"));

        for (String yeah : new JointIterable<String>(yeahs1,
                                                     null,
                                                     yeahs2,
                                                     yeahs3,
                                                     yeahs4)) {
            System.out.println(yeah);
        }
    }
}

JointIterableTest.java

package net.coderodde.util;

import java.util.Arrays;
import java.util.List;
import java.util.NoSuchElementException;
import org.junit.Test;
import static org.junit.Assert.*;

public final class JointIterableTest {

    private Iterable<String> jointIterable;

    @Test
    public void testNullIterableList() {
        jointIterable = new JointIterable<String>(null);    
        assertFalse(jointIterable.iterator().hasNext());
    }

    @Test(expected = NoSuchElementException.class)
    public void testNullIterableListThrowsOnNext() {
        new JointIterable<String>(null).iterator().next();
    }

    @Test
    public void testEmptyIterableList() {
        jointIterable = new JointIterable<String>();
        assertFalse(jointIterable.iterator().hasNext());
    }

    @Test(expected = NoSuchElementException.class)
    public void testEmptyIterableListThrowsOnNext() {
        new JointIterable<String>().iterator().next();
    }

    @Test
    public void testOmitsNullIterable() {
        List<String> list1 = Arrays.asList("one");
        List<String> list2 = Arrays.asList("two", "three");
        jointIterable = new JointIterable<String>(null, 
                                                  list1, 
                                                  null, 
                                                  null, 
                                                  list2, 
                                                  null);

        assertTrue(jointIterable.iterator().hasNext());
        assertEquals("one", jointIterable.iterator().next());

        assertTrue(jointIterable.iterator().hasNext());
        assertEquals("two", jointIterable.iterator().next());

        assertTrue(jointIterable.iterator().hasNext());
        assertEquals("three", jointIterable.iterator().next());

        assertFalse(jointIterable.iterator().hasNext());
    }
}
\$\endgroup\$
1
\$\begingroup\$

The easy stuff

Let's start with the easy stuff first:

  • Instead of creating a new ArrayList<>(0), it's better to use Collections.emptyList() when possible. This will avoid unnecessary object creation.
  • Similarly, if there are no (more) iterators to iterate, then instead of creating a new JointIterator(), it would be better to use Collections.emptyIterator().
  • Since you already filter out null iterators at the time of construction, I think it will also make sense to filter out empty iterators.

Based on the above, I would rewrite the constructor and iterator() like this:

  @SafeVarargs
  public JointIterable(Iterable<E>... iterables) {
    if (iterables == null) {
      this.iterators = Collections.emptyList();
    } else {
      this.iterators = Stream.of(iterables)
          .filter(Objects::nonNull)
          .map(Iterable::iterator)
          .filter(Iterator::hasNext)
          .collect(Collectors.toList());
    }
  }

  @Override
  @Nonnull
  public Iterator<E> iterator() {
    List<Iterator<E>> nonEmptyIterators = iterators.stream()
        .filter(Iterator::hasNext)
        .collect(Collectors.toList());
    return nonEmptyIterators.isEmpty()
        ? Collections.emptyIterator()
        : new JointIterator(nonEmptyIterators.iterator());
  }

Iterable and iterator behavior

Most iterables I know of can be iterated on multiple times. Normally if I'm given an Iterable<E>, I can call .iterator() on it multiple times and get the same values. The current implementation doesn't behave like that, once iterated over, it is consumed, further iterations will be empty. As I don't normally see this pattern, it might be surprising to others too.

Smelly hasNext

I think it's a code smell that hasNext mutates the state of JointIterator. It would be better to rework in a way that only next would change internal state.

Consider this alternative implementation of JointIterator:

  private final class JointIterator implements Iterator<E> {

    private final Iterator<Iterator<E>> iterators;
    private Iterator<E> iterator;

    private JointIterator(Iterator<Iterator<E>> nonEmptyIterators) {
      this.iterators = nonEmptyIterators;
      iterator = this.iterators.next();
    }

    @Override
    public boolean hasNext() {
      return iterator.hasNext();
    }

    @Override
    public E next() {
      E value = iterator.next();
      if (!iterator.hasNext() && iterators.hasNext()) {
        iterator = iterators.next();
      }
      return value;
    }
  }
\$\endgroup\$
  • \$\begingroup\$ I thought about a follow-up question, but that would be pretty much code plagiarism. What do you think on this matter? \$\endgroup\$ – coderodde Sep 17 '17 at 16:48
  • \$\begingroup\$ @coderodde I think it's not plagiarism when you give credit. So feel free to go ahead. \$\endgroup\$ – Stop ongoing harm to Monica Sep 17 '17 at 16:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.