CODEWITHSUNDEEP
java-8java-stream
Zymus
Zymus

Reputation: 1698

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

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?);
    });

Upvotes: 1

Views: 136

Answers (1)

Misha
Misha

Reputation: 28133

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.

Upvotes: 2

Related Questions