11
\$\begingroup\$

This is my first attempt at creating an event system.

My plan is that upon publishing, the listener receives both a reference to the model and to the publisher. This allows the publisher class to have multiple owners, and provide a way for listeners to know exactly how they were called.

To do this I've made the base classes for the publisher/listener generic

abstract class AEventPublisher<Model, Publisher extends AEventPublisher<Model, ?>> {
    private List<IEventListener<Model, Publisher>> listeners;

    public AEventPublisher(){
        listeners = new ArrayList<IEventListener<Model, Publisher>>();
    }

    public void subscribe(IEventListener<Model, Publisher> listener) {
        listeners.add(listener);
    }

    public void unsubscribe(IEventListener<Model, Publisher> listener) {
        listeners.remove(listener);
    }

    protected void publish(Publisher publisher, Model sender) {
        for(IEventListener<Model, Publisher> listener : listeners){
            listener.actionPerformed(publisher, sender);
        }
    }

    public abstract void publish(Model sender);
}

interface IEventListener<Model, EventPublisher extends AEventPublisher<Model, ?>> {
    public void actionPerformed(EventPublisher publisher, Model sender);
}

The publish method was made protected and abstract, because I couldnt think of a way to get this in AEventPublisher to be casted as the Publisher extends AEventPublisher<Model, ?> type


Some defined event/listeners would then be:

public class GameEvents{
    public static interface IButtonEventListener extends IEventListener<MenuButton, ButtonEventPublisher>{}

    public static class ButtonEventPublisher extends AEventPublisher<MenuButton, ButtonEventPublisher> {
        @Override
        public void publish(MenuButton sender) {
            publish(this, sender);
        }
    }

    public static interface IJoystickEventListener extends IEventListener<Joystick, JoystickEventPublisher> {}

    public static class JoystickEventPublisher extends AEventPublisher<Joystick, JoystickEventPublisher> {
        @Override
        public void publish(Joystick sender) {
            publish(this, sender);
        }
    }
}

Now the listener Interfaces can be turned into anonymous classes to act as callbacks.

This would be an example usage as part of the constructor for my Player class.

joystickCallback = new IJoystickEventListener(){
    @Override 
    public void actionPerformed(JoystickEventPublisher event, Joystick sender) 
    {
        updatePlayerDirection(sender);
    }
};
joystick.getJoystickEventPublisher().subscribe(joystickCallback);
//Then before player is deleted, this would be called:
//joystick.getJoystickEventPublisher().unsubscribe(joystickCallback);
\$\endgroup\$
12
+50
\$\begingroup\$

I've got a few small points about this code and then I have some other points regarding your design. I'll try to discuss your design in perspective to game-development (where I am completely inexperienced) and in perspective to (the more common Java area) business applications.

Names and Design choices

Your class (and interface) names seem to come from a C# dominated development perspective to me. Prefixing I to interfaces smells .NET to me. Whenever I am developing Java I try to keep interfaces without clutter and then usually suffix the implementation classes if that is required.

Next thing that somewhat bit me was the A prefix for your abstract class. I am not a great friend of abstract classes anyways. I try to avoid them wherever I can and prefer interfaces and "Default[...]"-implementations. I have been going better with that since abstract classes often end up constraining the use of the "Interface" too much.

I'd rather write a little duplicate code than creating a central abstract class that limits my possibilities when using the interface. Instead I'll create a simple, if not "minimalistic" interface. Multiple minimalistic interfaces can be combined to allow more complex uses, but since your use-case is clear ....

Anyways this is enough of abstract talk about personal preferences in regards to language constructs. Let's give your code a thorough examination.

Code Analysis:

What instantly struck me when I first saw your code was the lack of explicit visibility modifiers on your AEventPublisher and your IEventListener. Neither of these needs the other for context so much that you'd have to put them into the same classfile.

The next thing I saw was your incredibly long generics specification:

AEventPublisher<Model, Publisher extends AEventPublisher<Model, ?>>

This seems to be a lot of unnecessary work to me. Also you're needlessly restricting the possibilities you have, when using your publishing mechanism. IMO the following would be sufficient:

AEventPublisher<Model, P extends Publisher>

Then I saw how you store listeners. You have a private List<...>

You have two choices when creating an abstract class like this. Either you allow "full" access to your fields (by making them protected) or you close yourself off completely.

Following the Open/Closed principle (Open to extension, Closed to modification) I definitely suggest the latter. You did that not even bad, but I have two small nitpicks to make.

  1. Make your "immutable" fields really immutable:

    private final List<...>
    

    is better than just a private list.

  2. Make use of "common" language features:
    You're currently initializing your List in the constructor, and (again) repeat the clunky generics definition. Save yourself from that and use the Java 7 feature: the diamond operator. It makes overly clunky generics (as java has them) easier by shortcircuiting redundancy:

    private final List<IEventListener<Model, Publisher>> listeners = new ArrayList<>();
    

Next up is how you handle publishing. The problem you're describing when you implemented your publish method is rooted in how you restricted yourself when starting directly with an abstract class, instead of taking yourself the room of creating an interface for a Publisher. Consider the following:

public interface Publisher<E> {
    public void subscribe(IEventListener<E, Publisher<E>> listener);
    public void unsubscribe(IEventListener<E, Publisher<E>> listener);
    public void publish(E sender);
}

This greatly simplifies your generics declarations and nestings and additionally resolves the conflict you had internally.

If you'd sacrifice a little "flexibility" (which you problably won't need anyways, more later), you could even make your IEventListener much simpler and completely get rid of the second type-parameter you introduced.

In general your solution seems overengineered for the problem. Which brings me to my next point.

Design Discussion:

In general your design is somewhat over the top. You try to keep flexibility and allow substitions (which is in principle a good thing) in places where it's overly complicating your source-code and detrimental to code-readability and hurtful to your actual flexibility.

Coming from a "GameDevelopment-Perspective"

You will never need a Listener, which can listen to multiple different model classes, and if against all odds you need it, an abstract class or interface to group these model-classes is the simplest call to make. You went too far in your idealistic approach to be "open to everything". This is something you should strive to do, but not at the cost of simplicity and effectiveness.

This is true to both gamedevelopment and business applications.

In addition to that, gamedevelopment is a often heavily performance-reliant environment, and let me tell you, overcomplicated generics and backreferencing interfaces are not something to make the JVM run faster.

I also have no idea why you'd need or want the Listener to obtain a reference to the publisher. From what I see with my nonexistant experience you could "directly" subscribe to your model, which would then pass a reference to itself to it's listener. But that makes the whole Model-circus in your code moot, and defeats the point of a separate Publisher (which I don't see in the first place).

In general, separating the Publisher from the "published" is somewhat overkill and most probably sign of an underlying design mistake. I can not imagine a situation where I'd have to get a reference to a Publisher, before publishing something. You're again: overly complicating a simple matter.

Because in the end, what is this all about?

You want to bring data from one end to another, depending on certain conditions. And there's already a lot of existing solutions (beginning with Observable and ending god knows where) in the Java world for that.

Which is also what makes this model somewhat problematic from a business perspective. The simplicity of existing solutions beats your idea easily. The practical use-case is from my point of view missing.

Overall you reinvented the wheel, but not as wheel, but as a polygon. Your solution to "Get data from A to B on demand" suffers from clunkyness in use. It's a bumpy ride and I don't know how I'd make it better without landing at some place that's already polished for use.

Final overview:

When I reviewed your code I copied it to a local project in my IDE. A few changes and a little headscratching later I was at the state of code that follows.

From what your use-case seems to be, that should be completely sufficient:

public interface Publisher<E> {
    public void subscribe(IEventListener<E> listener);
    public void unsubscribe(IEventListener<E> listener);
    public void publish(E sender);
}

public interface IEventListener<E> {
    public void actionPerformed(Publisher<E> publisher, E sender);
}

public class DefaultEventPublisher<Model> implements Publisher<Model> {
    private final ArrayList<IEventListener<Model>> listeners = new ArrayList<>();

    @Override
    public void subscribe(IEventListener<Model> listener) {
        listeners.add(listener);
    }

    @Override
    public void unsubscribe(IEventListener<Model> listener) {
        listeners.remove(listener);
    }

    @Override
    public void publish(Model sender) {
        for (IEventListener listener : listeners) {
            listener.actionPerformed(this, sender);
        }
    }
}

And since you also had an example of a usecase:

public class ButtonEventPublisher 
       extends DefaultEventPublisher<ButtonEventPublisher.ButtonEvent> {
    public static class ButtonEvent {}
}

This code should in essence be able to do the same things that your code can do, but is IMO much simpler. Let me know what you think ;)

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

Vogel612 covered most of the style points, as well as the very appropriate challenge "is this worth doing".

But if you are going to do it....

An important idea to understand in writing this code well is that you have two very different concerns embedded in AEventPublisher. The first of these is a Publisher; there is also a manager of EventListeners. Things will be much clearer if we expose these abstractions

public interface Publisher<E> {
    public void publish(E sender);
}

public interface Observable<L> {
    void subscribe(L listener);
    void unsubscribe(L listener);
}

Notice that these generics are unconstrained - the concepts we are representing here are generic, so the types should express that. The complexity that is specific to this use case will be represented in the classes that implement the use case.

Speaking of which

interface EventListener<E, P> {
    void actionPerformed(P publisher, E event);
}

We simplify the EventListener as well.

As a side note, I'm not too keen on actionPerformed. "Action Performed" is a great (excusing the vagueness) name for an event, but it's wrong for an object method. I'd strongly prefer either onEvent -- for a generic handler -- or onEventHappened. The fact that we're anticipating generics does make good naming harder; but I think onActionPerformed would be more consistent with best practices.

At this point, we have the interfaces that we need to do something scary....

abstract static class EventPublisher
        < E
        , P extends EventPublisher<E,P>>
        implements
        Publisher<E>
        , Observable<EventListener<E,P>> {
    List<EventListener<E,P>> listeners;

    public void subscribe(EventListener<E,P> listener) {
        listeners.add(listener);
    }

    public void unsubscribe(EventListener<E,P> listener) {
        listeners.remove(listener);
    }

    @Override
    public void publish(E event) {
        for(EventListener<E,P> listener : listeners){
            listener.actionPerformed(self(),event);
        }
    }

    // NOTE: this should only be implement in final classes!
    abstract protected P self();

}

Disclaimer: crazy formatting for readability is a code smell -- a good code review should challenge you hard on whether this introduces unnecessary complexity.

So what the heck is this? Breaking it down, we're defining a class with two generic parameters. E is an event, which can be anything; P is... well, it's anything that extends the EventPublisher we are currently defining. The class also promises to implement the nice clean Publisher and Observable interfaces we defined earlier.

Through the generics, we're able to resolve the circular dependency; it's all syntactic sugar, so although we get turtled all the way down, the types are all going to be erased when the compiler goes to work.

Why are we doing this crazy thing?

The publish method was made protected and abstract, because I couldnt think of a way to get this in AEventPublisher to be casted as the Publisher extends AEventPublisher

Right -- we simply cannot use this in the base class and preserve the specialized behavior that we want. We beat with the seemingly self referential P parameter in the signature -- our specialized EventPublisher is going to provide an implementation of self() with the exact signature we need it to have for the EventListeners to do the right thing.

Let's try an example - I wanted my IDE to show me that things actually worked, so first I created some implementations to be used in the template specialization.

interface Joystick {
    int getDirection();
}

static class Game {
    static void updatePlayerDirection(Joystick joystick) {
        joystick.getDirection();
    }
}

That's enough to prove that the EventHandler is really handling the right type of event. But I also want to see that it can really distinguish the right kind of EventPublisher, so I add an extra interface that the JoystickEventPublisher itself is expected to implement.

interface BobNotifier {
    void notifyBob();
}

Clearly, I'm running low on creativity here.

OK, showtime - what does the JoystickEventPublisher look like?

final static class JoystickEventPublisher extends EventPublisher<Joystick, JoystickEventPublisher>
        implements BobNotifier {

    @Override
    public void notifyBob() {
        //...
    }

    // We're "allowed" to implement self(), because this class is final.
    protected JoystickEventPublisher self() {
        return this;
    }
}

JoystickEventPublisher extends EventPublisher<E,JoystickEventPublisher<E,P>> which means it really is a P that extends EventPublisher<E,P> and the compiler is satisfied!

A JoystickEventListener which subscribes to the JoystickEventPublisher really gets to use methods that are not part of the EventPublisher interface:

static class JoystickEventListener implements EventListener<Joystick, JoystickEventPublisher> {

    public void actionPerformed(JoystickEventPublisher publisher, Joystick joystick)
    {
        publisher.notifyBob();
        Game.updatePlayerDirection(joystick);
    }
};

And both subscription and publication "just work".

    JoystickEventPublisher p = new JoystickEventPublisher();
    p.subscribe(new JoystickEventListener());
    p.publish(new Joystick() {
        @Override
        public int getDirection() {
            return 0;
        }
    });

A couple of further points. The "abstract self()" pattern shows up in other contexts -- it's very useful for implementing fluent builders. So you might consider teasing that piece out.

abstract static class AbstractSelf<T extends AbstractSelf<T>> {
    // NOTE: this should only be implement in final classes!
    abstract protected T self();
}

abstract static class EventPublisher
        < E
        , P extends EventPublisher<E,P>>
        extends 
        AbstractSelf<P>
        implements
        Publisher<E>
        , Observable<EventListener<E,P>> {
        //...
}

Which is nice, clean, re-usable.... It works great when you are building a new class from scratch. However, if you are trying to extend some other class, the language won't allow you to extend a second class, and you'll have to re-write the self() declaration anyway. It may not be worth the effort.

Second - you may not want to restrict the kinds of Listeners are attached to EventPublishers. For instance, you might have a generic listener that wants to subscribe to joystick events and keyboard events. We can support that here by loosening the constraints on the listeners. That's going to change the Observer part of the interface while leaving everything else alone -- another hint that observe and publish are two separate concerns.

abstract static class EventPublisher
        < E
        , P extends EventPublisher<E,P>>
        implements
        Publisher<E>
        , Observable<EventListener<? super E, ? super P>> {
    List<EventListener<? super E, ? super P>> listeners;

    public void subscribe(EventListener<? super E, ? super P> listener) {
        listeners.add(listener);
    }

    public void unsubscribe(EventListener<? super E, ? super P> listener) {
        listeners.remove(listener);
    }

    @Override
    public void publish(E event) {
        for(EventListener<? super E, ? super P> listener : listeners){
            listener.actionPerformed(self(),event);
        }
    }

    // NOTE: this should only be implement in final classes!
    abstract protected P self();

}

The compiler is happy enough....

    p.subscribe(new EventListener<Object, Object>() {
        @Override
        public void actionPerformed(Object publisher, Object event) {
            //...
        }
    });

Finally, you may find that you want to extend JoystickEventPublisher. In the implementation above, the class was deliberately declared final to close that door, because the definition of self() provided by JoystickEventPublisher is going to prevent the event listeners from accessing any new methods in ExtendedJoystickEventPublisher. The right answer is to refactor the JoystickEventPublisher into two pieces - an abstract and extendable piece that has all of the interesting parts of the interface, and a final concrete piece that implements self()

abstract static class AbstractJoystickEventPublisher<P extends AbstractJoystickEventPublisher<P>> extends EventPublisher<Joystick, P>
        implements BobNotifier {

    @Override
    public void notifyBob() {
        //...
    }

}

final static class JoystickEventPublisher extends AbstractJoystickEventPublisher<JoystickEventPublisher> {
    // We're "allowed" to implement self(), because this class is final.
    protected JoystickEventPublisher self() {
        return this;
    }
}

This implementation allows us to create a kind of JoystickEventPublisher which shares the implementation of the BobNotifier interface in addition to providing new methods....

abstract static class AbstractEnhancedJoystickEventPublisher<P extends AbstractEnhancedJoystickEventPublisher<P>> extends AbstractJoystickEventPublisher<P> {
    public void notifyAlice() {
        // ...    
    }    
}

final static class EnhancedJoystickEventPublisher extends AbstractEnhancedJoystickEventPublisher<EnhancedJoystickEventPublisher> {

    protected EnhancedJoystickEventPublisher self() {
        return this;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Small mobile codereview review if you don't mind: I personally prefer to not break a 'level' of <> in generics definitions by adding a linebreak. Neither do I put implements and extends on separate lines. I think your generics formatting is overkill and harmful to clarity. And a small note to inner classes for listeners: a lambda like (obj0, obj1) -> {} should do the trick :) But aside from these rather nitpickish points: nice review \$\endgroup\$ – Vogel612 Mar 17 '15 at 23:28
  • \$\begingroup\$ In code, I agree -- here, I made a deliberate choice to break the declaration up into multiple lines to avoid horizontal scrolling in the key class in this presentation. (An earlier draft had a generic parameter for the listener as well, which made the single line declaration more difficult to read). Hence my comment: this implementation may be too intricate to be maintainable. Agreed on the lambda. The anonymous class was used in the question; it seemed appropriate to demonstrate that form. \$\endgroup\$ – VoiceOfUnreason Mar 18 '15 at 2:28

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.