Firstly, as usual, this answer assumes anyone reading this already saw Simon's answer.
MVC Architecture Issues
The problem with this design:
public interface HandView extends View<Hand> {
//... some handler methods
}
apart from being just confusing, is that it violates Interface Segregation. By violating this principle, namely that identifiable roles map to corresponding small interfaces, it forces the publisher of these events to depend on HandView and everything else it depends on. As a publisher/generator of some events I do not care if the listeners are views or loggers or statistic aggregators analyzers or just no-ops etc.
Imagine for example, that I want to dim the cards in the hand view when turn passes to the other player. This feature requires that HandView also listens to some onNextTurn(), or onTurnPassed(Player nextPlayer) events. For example I am editing the Hand model class and want to raise some event. If I type handView. the IDE tells me I also have access to onTurnPassed(Player nextPlayer). This not only means Hand depends on Player unnecessarily, it should not be able to raise such events.
You are trying to do two things at once, which makes it also SRP violation: First it is a design contract for a View Component. It also should be the role contract for any component that listens to Hand Events. We generally need the first kind of interfaces for abstract factories, e.g. HandView handView = createHandView(). I need to make sure HandView interface can fill all roles a Hand View is supposed to fill, if I am to avoid unchecked casts etc non-type-safe operations.
You can refactor it thus:
interface HandView extends HandListener, TurnListener, ..... {
// Nothing
// this is a design contract for Hand View component
// I can use this interface as return type of an abstract factory
// or to test features (functional test)
// that involves multiple roles of a Hand View
// Better still I can just delete this if I'm not doing either
}
interface HandListener {
//... handler methods for hand events
}
Per-event Repetition
Another problem point is per-event repetition, but before we go there let's see some other code smells, even though not as prominent, are more easily identifiable, and upon alleviation will help us deal with this problem:
@FunctionalInterface
public static interface OnCardPlayedListener extends HandView, IntConsumer {
// ....
@Override
default void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo) { }
First of all this is a clearly identifiable case of Refused Bequest. default methods are supposed to be used for adding new behavior to existing interfaces, but here it is used to refuse some behavior of the super type. As said in the link if a subtype is refusing some behavior it may not be a serious problem but if it is refusing the interface of the extended type it is serious. Any new behavior added to HandView would be a shotgun surgery to types that extend it. CardPlayedListener extends HandView means a CardPlayedListener is a HandView; whereas, in fact it is just the opposite, a View of a Hand is a listener of Card-Played event. This is not, as you call it, 'logically correct'.
Let's also look at the signatures of the methods: we have a void(Card), which isn't that bad, but the others are void(int), void(int, int). Even if this fact would not be an absolute indication for a refactoring; any model layer concerns, such as domain events should use domain types as parameters as much as possible. Suppose you decided to pass the Card played along with the cardIndex to the onCardPlayed methods, you would have to change all the handler code, even though they are working perfectly. With Java 8 adding a new method to an interface need not be a breaking change, but adding a parameter still is.
Let's now look at CardSwapped more closely, void (int cardIndexOne, int cardIndexTwo). It is clear that cardIndexOne, cardIndexTwo are some kind of Data Clump. They don't mean anything on their own. They have some rules implicit or explicit that about them which means they are not ordinary integers. For example indexes can't be negative and if hand size is constant should be less than HAND_SIZE. Since a swap cards (n, n) type no-op would not trigger a CardSwapped event in the model, they probably should not be same etc. These kind of observations are indications that they are a part of some domain type.
After all this talk, good news, our job is so easy: In case of domain events those domain types are Domain Events themselves! Just apply Introduce Parameter Object, and were done.
class CardAdded {
Card card;
}
class CardPlayed {
int cardIndex;
}
class CardsSwapped {
int cardIndexOne;
int cardIndexTwo;
}
onCardAdded(CardAdded) is redundant so let's rename them to void on(CardAdded). now it is more clear that a listener is some kind of consumer for the event.
@FunctionalInterface
interface Listener<T> {
void on(T event);
}
Because of type erasure we cannot say of course HandListener extends Listener<CardAdded>, Listener<CardPlayed>, .... But think of this interface somewhat like a type-safe marker, a thing pre-generics java.util.EventListener could not be. Also note that HandListener does not listen to Hand Events as a CardAddedListener listener listens to CardAdded. Which is a possibly extensible logical grouping of events, the other is is a single specific event.
Composition-over-inheritence HandListener:
public interface HandListener {
Listener<CardAdded> onCardAdded();
Listener<CardAdded> onCardPlayed();
Listener<CardAdded> onCardsSwapped();
}
onCardAdded() etc are just getters but I did not named them getCardAddedHandler, this way it is less wordy and less framework-y and more domain-y. This also make clear that they are not meant to be called like handListener.onCardAdded().oc(cardAdded), and instead like hand.onCardAdded().addListener(handView.onCardAdded()) in a similar way to Java Observable and .NET events.
I doubt you would need merge method at this point, but you can use something like this instead to chain generic interfaces, which cannot be used in varargs :
default Listener<T> andThen(Listener<T> otherListener) {
return event -> {
this.on(event);
otherListener.on(event);
};
}
Add/Remove Listener
I refactored the test case as below to make it compile with the changes I suggested:
@Test
public void shouldNotTriggerCallbackAfterRemoval() {
// give them names indicating we don't care what they actually are
MonsterCard someCard = new MonsterCard("Test", 7, 7, MonsterModus.HEALING);
MonsterCard someOtherCard = new MonsterCard("Test", 5, 5, MonsterModus.HEALING);
MonsterCard yetAnotherCard = new MonsterCard("Test", 3, 3, MonsterModus.HEALING);
Hand hand = new Hand(5);
AtomicInteger counterOne = new AtomicInteger(0);
Listener<CardAdded> listenerOne = cardAdded -> counterOne.incrementAndGet();
AtomicInteger counterTwo = new AtomicInteger(0);
Listener<CardAdded> listenerTwo = cardAdded -> counterTwo.incrementAndGet();
hand.onCardAdded().addListener(listenerOne);
hand.onCardAdded().addListener(listenerTwo);
hand.add(someCard);
assertEquals(1, counterOne.get());
assertEquals(1, counterTwo.get());
hand.onCardAdded().removeListener(listenerTwo);
hand.add(someOtherCard);
assertEquals(2, counterOne.get());
assertEquals(1, counterTwo.get());
hand.onCardAdded().removeListener(listenerOne);
hand.add(yetAnotherCard);
assertEquals(2, counterOne.get());
assertEquals(1, counterTwo.get());
}
I split the counters this way because it will makes sure the correct listener is removed.
If you only assert on the total number of listener invocations, removing any listener in
removeListener will make it pass!
Still IDE is complaining I'm not using the event parameter in the handlers and it is right.
If I'm calling the listeners with null or any other value regardless of which card is added the test would still pass. If you follow the "Write just enough code to make the test pass." maxim, you will catch this kind of cases. Maybe you could use something like AtomicReference<CardAdded> lastParameterOne, and assert on it.
I added these to make it pass; so names, design etc is just to give you an idea :
interface EventSource<T> {
void addListener(Listener<T> listener);
void removeListener(Listener<T> listener);
}
interface Notifier<T> {
void fire(T event);
}
interface Publisher<T> extends EventSource<T>, Notifier<T> {
}
class SimpleEvent<T> implements Publisher<T> {
private final List<Listener<T>> listeners = new ArrayList<>();
@Override
public void fire(T event) {
listeners.forEach(l -> l.on(event));
}
@Override
public void addListener(Listener<T> listener) {
listeners.add(listener);
}
@Override
public void removeListener(Listener<T> listener) {
listeners.remove(listener);
}
}
class Hand {
public void addCard(Card card) {
// ....
fireCardAdded(card);
}
private void fireCardAdded(Card card) {
onCardAdded().fire(new CardAdded(card));
}
private final Publisher<CardAdded> cardAdded = new SimpleEvent<>();
public Publisher<CardAdded> onCardAdded() {
return cardAdded;
}
}
