HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Event Listener and Publisher

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andpublisherlistenerevent

Problem

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> {
    private List> listeners;

    public AEventPublisher(){
        listeners = new ArrayList>();
    }

    public void subscribe(IEventListener listener) {
        listeners.add(listener);
    }

    public void unsubscribe(IEventListener listener) {
        listeners.remove(listener);
    }

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

    public abstract void publish(Model sender);
}

interface IEventListener> {
    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 type

Some defined event/listeners would then be:

public class GameEvents{
    public static interface IButtonEventListener extends IEventListener{}

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

    public static interface IJoystickEventListener extends IEventListener {}

    public static class JoystickEventPublisher extends AEventPublisher {
        @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

Solution

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>


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


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.

-
Make your "immutable" fields really immutable:

private final List


is better than just a private list.

-
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> 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 {
    public void subscribe(IEventListener> listener);
    public void unsubscribe(IEventListener> 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" subscrib

Code Snippets

AEventPublisher<Model, Publisher extends AEventPublisher<Model, ?>>
AEventPublisher<Model, P extends Publisher>
private final List<...>
private final List<IEventListener<Model, Publisher>> listeners = new ArrayList<>();
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);
}

Context

StackExchange Code Review Q#84037, answer score: 12

Revisions (0)

No revisions yet.