10
\$\begingroup\$

I just created the following code and am wondering whether it is logically correct (all other feedback is of course also welcome):

public interface HandView extends View<Hand> {
    void onCardAdded(final Card card);

    void onCardPlayed(final int cardIndex);

    void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo);

    public static HandView merge(final HandView... handViews) {
        return new HandView() {
            @Override
            public void onCardAdded(final Card card) {
                Arrays.stream(handViews).forEach(handView -> handView.onCardAdded(card));
            }

            @Override
            public void onCardPlayed(final int cardIndex) {
                Arrays.stream(handViews).forEach(handView -> handView.onCardPlayed(cardIndex));
            }

            @Override
            public void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo) {
                Arrays.stream(handViews).forEach(handView -> handView.onCardsSwapped(cardIndexOne, cardIndexTwo));
            }
        };
    }

    @FunctionalInterface
    public static interface OnCardAddedListener extends HandView, Consumer<Card> {
        @Override
        default void accept(final Card card) {
            onCardAdded(card);
        }

        @Override
        void onCardAdded(final Card card);

        @Override
        default void onCardPlayed(final int cardIndex) { }

        @Override
        default void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo) { }
    }

    @FunctionalInterface
    public static interface OnCardPlayedListener extends HandView, IntConsumer {
        @Override
        default void accept(final int cardIndex) {
            onCardPlayed(cardIndex);
        }

        @Override
        default void onCardAdded(final Card card) { }

        @Override
        void onCardPlayed(final int cardIndex);

        @Override
        default void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo) { }
    }

    @FunctionalInterface
    public static interface OnCardsSwappedListener extends HandView {
        @Override
        default void onCardAdded(final Card card) { }

        @Override
        default void onCardPlayed(final int cardIndex) { }

        @Override
        void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo);
    }
}

And then I used it like this in the following integration test (so you can see the usage):

@Test
public void testRemoveCallbackNotTriggeredAfterRemoval() {
    Hand hand = new Hand(5);
    AtomicInteger counter = new AtomicInteger(0);
    HandView.OnCardAddedListener handView = card -> counter.incrementAndGet();
    hand.addViewCallback(handView);
    HandView.OnCardAddedListener handView2 = card -> counter.incrementAndGet();
    hand.addViewCallback(handView2);

    hand.add(new MonsterCard("Test", 7, 7, MonsterModus.HEALING));
    assertEquals(2, counter.get());

    hand.removeViewCallback(handView2);
    hand.add(new MonsterCard("Test Test", 5, 5, MonsterModus.HEALING));
    assertEquals(3, counter.get());

    hand.removeViewCallback(handView);
    hand.add(new MonsterCard("Test Test Test", 3, 3, MonsterModus.HEALING));
    assertEquals(3, counter.get());
}

Where Hand.addViewCallback expects an argument of type HandView.

It seems to be working, the only thing I am left off wondering is:

  • Is it not normal to have HandView extends OnCardAddedListener, OnCardPlayerListener, OnCardsSwappedListener?

The caveat then is that I don't think methods accepting an HandView now, would accept those sub-interfaces then.

\$\endgroup\$
4
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Absolutely brilliant! Your reminded me about why I use domain-specific Event classes in my own code. Perhaps I should have said this to skiwi a couple of days ago. Thanks for reminding me! And also thanks for teaching me some more stuff! \$\endgroup\$ – Simon Forsberg May 5 '14 at 15:08
  • 1
    \$\begingroup\$ @SimonAndréForsberg Thanks for the kind remarks. I'm glad my post has been helpful to you in some way. Many good practices become muscle memory for experienced programmers, but I'm keeping things extra-pedantic and addressing not the regular users of CR but my past, fresh-out-of-school self. \$\endgroup\$ – abuzittin gillifirca May 6 '14 at 7:09
5
\$\begingroup\$

Honestly, skiwi, you make things more complicated than they need to be.

Please rename View to Listener to avoid confusion. Because that's really what they are, it's a Listener, not a View. It doesn't by itself provide any UI functionality.

Prefixing your interfaces with On is, although understandable why you are doing it, not something I recommend or have seen been done in other places. You're already suffixing Listener to the name (which is what I would recommend, and what I have seen in lots of other code), no need to prefix the interface with On. The method names can be prefixed with on, as they have been already.

Why are you bringing IntConsumer and Consumer<Card> into your code? In what way does that help you? In my opinion, it doesn't help you at all. It creates more confusion than brings something good. If you want to "transform" an IntConsumer into a CardPlayedListener, use Java 8's method reference style by using something like IntConsumer::accept.

Now, let's see if we can write more or less the same thing of your interface heirarchy but with fewer lines.

public interface HandListener {
    default void onCardAdded(final Card card) {}

    default void onCardPlayed(final int cardIndex) {}

    default void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo) {}

    public static HandListener merge(final HandListener... handViews) {
        return new HandListener() {
            @Override
            public void onCardAdded(final Card card) {
                Arrays.stream(handViews).forEach(handView -> handView.onCardAdded(card));
            }

            @Override
            public void onCardPlayed(final int cardIndex) {
                Arrays.stream(handViews).forEach(handView -> handView.onCardPlayed(cardIndex));
            }

            @Override
            public void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo) {
                Arrays.stream(handViews).forEach(handView -> handView.onCardsSwapped(cardIndexOne, cardIndexTwo));
            }
        };
    }

    @FunctionalInterface
    public static interface CardAddedListener extends HandListener {
        @Override
        void onCardAdded(final Card card);
    }

    @FunctionalInterface
    public static interface CardPlayedListener extends HandListener {
        @Override
        void onCardPlayed(final int cardIndex);
    }

    @FunctionalInterface
    public static interface CardsSwappedListener extends HandListener {
        @Override
        void onCardsSwapped(final int cardIndexOne, final int cardIndexTwo);
    }
}

This way, you can keep having only HandListeners as listeners in your Hand, no need to split it up into three separate listeners. And if you want to easily create a listener that listens only to cards being swapped, you can easily do so. The trick here is that I'm overriding a default method and removing the implementation, thus making it abstract again. While letting the original HandListener interface have default implementations for all the methods.

  • Original lines of code: 72
  • New lines of code: 43
  • Lines of code reduced by: 40%
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.