Dungeon: do not throw exceptions in stream operations (refactoring, SpikeSystem)

[cagix]

see #1564 (comment)

In

public void execute() {
     filteredEntityStream(SpikyComponent.class)
         .forEach(e -> e.fetch(SpikyComponent.class).orElseThrow().reduceCoolDown());
}

wird ein Stream aller Entitäten mit einer SpikyComponent vom System angefordert. Bei der Iteration über diese Entitäten wird dann (a) die SpikyComponent extrahiert, (b) eine Exception geworfen im Nichtvorhandensein-Fall, und (c) auf der Component eine Methode aufgerufen.

Dies ist aus zwei Gründen problematisch: Zum werden mehrere Operationen in die forEach-Schleife verpackt, und zum anderen sollte man keine Exceptions in einer Stream-Operation werfen. (OK, das dürfte hier in der Realität auch nie passieren, da wir ja vorher exakt nach der Component gefiltert hatten. Dennoch.)

Dieser PR führt ein Refactoring durch und erledigt in jedem Stream-Step eine Sache. Außerdem wird keine Exception mehr in der Stream-Verarbeitung geworfen, sondern Entitäten, die zwischen System#filteredEntityStream und dem forEach auf geheimnisvolle Weise ihre SpikyComponent verloren haben sollten, werden einfach ignoriert.

Keine Änderung der API und/oder des Verhaltens.

[cagix]

@AMatutat Ich habe nochmal map und flatMap gesplittet - ist das lesbarer als die vorige Variante?

    filteredEntityStream(SpikyComponent.class)
        .flatMap((e -> e.fetch(SpikyComponent.class).stream()))
        .forEach(SpikyComponent::reduceCoolDown);

vs.

    filteredEntityStream(SpikyComponent.class)
        .map((e -> e.fetch(SpikyComponent.class)))
        .flatMap((Optional::stream))
        .forEach(SpikyComponent::reduceCoolDown);

Meinungen?

[AMatutat]

    filteredEntityStream(SpikyComponent.class)
        .map((e -> e.fetch(SpikyComponent.class)))
        .flatMap((Optional::stream))
        .forEach(SpikyComponent::reduceCoolDown);

Aus einem Studi gezielten Blickwinkel finde ich Nummer 2 besser. Da wird die Optional geschichte nochmal sichtbar.

[cagix]

ja, finde ich auch. flatmap sagt zwar auch schon alles an sich, aber das extra .stream() ist typisch java - die java-designer haben vermutlich im meeting gesessen und gedacht "lasst mal sehen, was ist die bescheuertste ausdrucksform? supi, das nehmen wir so." 🫣🤣

[cagix]

@AMatutat Noch eine andere Variante:

(A)

    filteredEntityStream(SpikyComponent.class)
        .map(e -> e.fetch(SpikyComponent.class))
        .forEach(sc -> sc.ifPresent(SpikyComponent::reduceCoolDown));

vs. Vorschlag (2) von oben

(B)

    filteredEntityStream(SpikyComponent.class)
        .map(e -> e.fetch(SpikyComponent.class))
        .flatMap((Optional::stream))
        .forEach(SpikyComponent::reduceCoolDown);

Im Grunde führen .flatMap((Optional::stream)) und sc.ifPresent() zum selben Ergebnis: Das Optional aus dem e.fetch(SpikyComponent.class) wird ausgepackt und falls vorhanden mit SpikyComponent::reduceCoolDown behandelt.

Meinungen? Eine dieser beiden Varianten würde ich tatsächlich einchecken, d.h. aus meiner Sicht sind beide besser als das Original.

[cagix]

@AMatutat Präferenzen? (A) oder (B)?

[cagix]

klammerpaar zuviel/doppelt

[cagix]

keine reaktion von @AMatutat ... going w/ (b).