Rework BatchGameEvent and subclasses to deduplicate code and logic

[xenohedron]

Currently BatchGameEvent is just an interface with two methods.

public interface BatchGameEvent<T extends GameEvent> {
    Set<T> getEvents();
    Set<UUID> getTargets();
}

However, the batch events that implement this interface use the same logic for the methods and also duplicate code for supplying and overriding other methods.

I propose making BatchGameEvent a class implementation extending GameEvent and including all the logic common to batch events.

A separate but related problem is DamagedBatchEvent which currently includes both general batches (any target/source) and batches where all events are associated with the same target. Since these have different usage patterns it might be sensible to separate that out as well.

That would give us something like this:

• BatchGameEvent
  • MultiTargetBatchEvent
    • DamageMultiBatchEvent (player/permanent)
    • TappedBatchEvent, etc.
  • SingleTargetBatchEvent
    • DamageSingleBatchEvent (player/permanent)

Related: #11974 , #11841, #11961, #11943 , #11943 (comment)

Comments welcome @JayDi85 @jimga150

[JayDi85]

Batch events should be as different as possible from normal events. It's important to use different logic for cards:

• "single event" must use standard methods: getTargetId, getPlayerId, etc;
• "multiple event" must use list methods: getEvents, getTargets, getPlayers, etc.

No needs to simulate single event methods in multiple events (example: getTargetId for events with same target) -- that's why I recommended to rename it in related PR.

So:

1. Refactor BatchGameEvent to abstract class with completed getEvents, getTargets and IllegalStateException in single methods;
2. Delete DamagedBatchEvent event at all -- no needs in damageClazz field, there are only two possible events: DamagedPlayerEvent and DamagedPermanentEvent.
3. MultiTargetBatchEvent and SingleTargetBatchEvent can be used as runtime check (example: collect players ids and raise error on size > 1 in SingleTargetBatchEvent -- same check for "add event" and other methods). No needs in other intermediate class like MultiPlayerBatchEvent, MultiSourcesBatchEvent, etc.

[jimga150]

I think i understand what you're saying @JayDi85 but just making sure: It sounds like you're saying this is the hierarchy we should pursue:

• BatchGameEvent
  • MultiTargetBatchEvent (getEvents, getTargets, getPlayers)
    • DamageMultiPlayerEvent
    • DamageMultiPermanentEvent
    • TappedBatchEvent, etc.
  • SingleTargetBatchEvent (getTargetId, getPlayerId)
    • DamageSinglePlayerEvent
    • DamageSinglePermanentEvent

Though i'm not sure what your point 3 is saying--that MultiTargetBatchEvent and SingleTargetBatchEvent would never be fired, and only used as subclasses for their validity checks as events are created?

Cause if we're just going to do that, and these classes will be similar enough, i guess we wouldn't need the specific classes at all and just use generic GameEvent instances with the existing pattern of just instantiating one with the enum. However that will be a problem since the multi-target batch events provide the antipattern detection when something like getTargetId is called. I'm thinking it will be valuable to allow people to receive their events as specific subclasses they can cast to.

[xenohedron]

How about a boolean parameter in the abstract BatchGameEvent class for whether there are multiple targets? Set in constructor by the implementing subclasses. Then the getTargetId can check that param and throw exception only if misaligned. Fewer classes needed.

[JayDi85]

Though i'm not sure what your point 3 is saying--that MultiTargetBatchEvent and SingleTargetBatchEvent would never be fired, and only used as subclasses for their validity checks as events are created?

Yep. I don't like all that intermediate classes at all. But it required here to extract some shared code for single and multiple methods/checks.

It's not such a big deal to find an ideal refactor -- just use it as you want. The most important thing -- remove all buggy group events and replace it by batches (looks like only ZONE_CHANGE_GROUP left).