What is a better way to iterate through two streams and apply operation to one

Question

If anyone can think of a better way to word it, I'd appreciate an edit for the title.

I have a class that represents a collection with a capacity.

The code I have is

public class PlayerParty implements Party {

    public PlayerParty() {
        this(Collections.emptyList());
    }

    public PlayerParty(Collection<Pokemon> pokemon) {
        Objects.requireNonNull(pokemon, "pokemon must not be null");
        if (pokemon.size() > PARTY_LIMIT) {
            throw new IllegalArgumentException(String.format(PARTY_LIMIT_EXCEEDED, PARTY_LIMIT));
        }

        createPartySlots();
        fillPartySlots(pokemon);
    }

    @Override
    public final Iterable<Pokemon> getPokemon() {
        return Collections.unmodifiableCollection(
               partySlots
                   .stream()
                   .filter(PartySlot::isFull)
                   .map(PartySlot::getPokemon)
                   .collect(Collectors.toList()));
    }

    public final Optional<PartySlot> getNextSlot() {
        return partySlots
                   .stream()
                   .filter(slot -> !slot.isFull())
                   .findFirst();
    }

    private void createPartySlots() {
        for (int i = 0; i < PARTY_LIMIT; i++) {
            partySlots.add(new PartySlot());
        }
    }

    private void fillPartySlots(Iterable<Pokemon> pokemon) {
        pokemon.forEach(p -> {
            // Since we just added all of the slots, they're
            // guaranteed to be present
            // noinspection OptionalGetWithoutIsPresent
            PartySlot slot = getNextSlot().get();
            slot.fill(p);
            partySlots.add(slot);
        });
    }

    private static final String PARTY_LIMIT_EXCEEDED = "party cannot have more than %s Pokemon";
    private static final int PARTY_LIMIT = 6;
    private final List<PartySlot> partySlots = new ArrayList<>();
}

The method in question revolves around fillPartySlots.

On the line PartySlot slot = getNextSlot().get();, I get a warning indicating I'm calling get without a call to isPresent first. This is understandable, as typically I would want to do that before trying to get a value out of the Optional.

Is there a better way to do perform an operation on one stream, based on the state of another stream? Namely, can I change this so that it uses isPresent (which should always be true in this method because the previous call in the constructor creates them)? Could I do something like

partySlots
    .stream()
    .filter(slot -> !slot.isFull()) // should return true for all slots
    .forEach(slot -> {
        slot.fill(nextAvailableOneFromMethodParameter?);
    });

score 2 · Accepted Answer · edited Apr 13 '17 at 12:40

2

It's OK to call .get() without checking .isPresent() if the case of the absent value represents a bug. If the value is absent, .get() will fail fast with a NoSuchElementException, which is what you probably want.

You may wish to document that more clearly in your code by using .orElseThrow(...) instead of .get(), but that's a matter of preference.

On the other hand, your proposed code with .filter(slot -> !slot.isFull()) will silently ignore the case of the slot being full and make such bug harder to find. Also, the alternate implementation iterates over slots rather than over the supplied pokemon, but what if there are fewer pokemon than there are slots?

As an aside, do you have a good reason to use Iterable? Collection or List is probably a more appropriate choice. Consider posting this to code review.

edited Apr 13 '17 at 12:40

Community

1
1

answered Apr 17 '16 at 05:36

Misha

27,433
6
62
78

If there are fewer pokemon than there are slots, than slots without one are fine. The idea is to have a max of 6 slots. A minimum of 1 slot must be filled. So one could have 3 pokemon, and the capacity to have 3 more. – Zymus Apr 17 '16 at 05:45
1

Right, so when you do a `.forEach` on the slots and try to fill them from the `Iterable` you will have to handle that case. – Misha Apr 17 '16 at 05:51
That was part of the question; how I could simultaneously iterate over both of them at the same time, and add each from the `Iterable` to the slots. – Zymus Apr 17 '16 at 05:55
1

Your current solution is fine. You might want to look at http://stackoverflow.com/q/17640754/3920048 for various ways to "zip" two streams but it is not worth the trouble for your case. – Misha Apr 17 '16 at 06:00

What is a better way to iterate through two streams and apply operation to one

1 Answers1