3
\$\begingroup\$

I am currently programming a game in java, and I recently implemented a centralized event dispatcher system in order to decrease coupling between different parts of my game.

My system consists of three classes : GameEventDispatcher, which contains the actual dispatching functionality, GameEventListener, a simple listener, and GameEvent, which is a basic object containing source and type of event. I think the only requirements I (currently) have are that events should be dispatched FIFO (First In First Out) and that the public methods should be allowed to be called from any thread and concurrently.

GameEventDispatcher :

//Imports omitted for clarity. All external classes used are part of the standard java 1.8 package

public class GameEventDispatcher {

    private static final GameEventDispatcher ourInstance = new GameEventDispatcher(); //Singleton pattern
    private static final Logger log = Logger.get(); //Internal logging utility
    private final Queue<GameEvent> eventsQueue = new ConcurrentLinkedQueue<>();
    private final Map<GameEvent.Type, List<GameEventListener>> typeListenersMap = new HashMap<>();
    private final Object waitingForEventsLock = new Object(); //Allows to wait until events are queued
    private boolean waitingForEvents = false; //Set to true when waiting
    private boolean doDispatch = true;

    public GameEventDispatcher() {

        new Thread(() -> {

            while (doDispatch) {
                try {
                    GameEvent e = eventsQueue.poll();

                    if (e == null) //Start waiting
                        synchronized (waitingForEventsLock) {
                            waitingForEvents = true;
                            waitingForEventsLock.wait();
                        }
                    else
                        dispatch(e);

                } catch (Exception e) {
                    log.e("Uncaught exception :");
                    e.printStackTrace();
                }
            }
        }, "Thread-Dispatcher").start();
    }

    public static GameEventDispatcher getInstance() {
        return ourInstance;
    }

    public void register(GameEvent.Type type, GameEventListener listener) {

        synchronized (typeListenersMap) {
            List<GameEventListener> list = typeListenersMap.computeIfAbsent(type, k -> new ArrayList<>());

            list.add(listener);
        }
    }

    public void unregister(GameEvent.Type type, GameEventListener listener) {

        synchronized (typeListenersMap) {
            List<GameEventListener> list = typeListenersMap.get(type);

            if (list == null || !list.remove(listener))
                throw new IllegalArgumentException("Trying to unregister a non-registered listener");
        }
    }

    public void submit(Object source, GameEvent.Type type) {

        submit(new GameEvent(source, type));
    }

    public void submit(GameEvent e) {

        if (e == null)
            throw new IllegalArgumentException("Event cannot be null");

        eventsQueue.add(e);

        if (waitingForEvents) //Stop waiting
            synchronized (waitingForEventsLock) {
                waitingForEvents = false;
                waitingForEventsLock.notify();
            }
    }

    private void dispatch(GameEvent e) {

        assert e != null; //Since this method is private, that should never happen

        synchronized (typeListenersMap) {

            List<GameEventListener> list = typeListenersMap.get(e.eventType);

            if (list != null)
                list.forEach(listener -> listener.executeEvent(e));
        }
    }
}

GameEvent :

public class GameEvent {

    public final Object source;
    public final Type eventType;

    public GameEvent(Object source, Type eventType) {
        this.source = source;
        this.eventType = eventType;
    }

    public enum Type {

        GAME_START,
        GAME_EXIT,
        GAME_PAUSE,
        GAME_RESUME,
        GAME_RESET,
        GAME_DEATH,
        CONFIG_RESET,
        KEYBINDINGS_RESET
    }
}

GameEventListener :

public interface GameEventListener {

    void executeEvent(GameEvent e);
}

I'd like to hear your overall opinion and suggestions about my code, however I also have some specific questions / doubts :

  • Is the use of the singleton pattern appropriate ?
  • Is having a specific thread (AWT-style) overkill or simply not appropriate ?
  • Is a ConcurrentLinkedQueue the right FIFO Queue implementation to go with ?
\$\endgroup\$
  • \$\begingroup\$ The Singleton Pattern is rarely thought to be appropriate. Introducing interfaces passed around, you usually don't need it. \$\endgroup\$ – πάντα ῥεῖ Jan 2 '17 at 16:03
  • \$\begingroup\$ @πάνταῥεῖ About the Singleton Pattern, I am aware it is generally not an appropriate design, however I am rather asking about its use in this specific context. Since here only needs to be one event dispatcher in the program, doesn't that make it a good candidate for a Singleton ? And I don't really understand what you meant with interfaces passed around. \$\endgroup\$ – Niss36 Jan 2 '17 at 16:09
  • \$\begingroup\$ What if other event sources should be introduced? \$\endgroup\$ – πάντα ῥεῖ Jan 2 '17 at 16:14
  • \$\begingroup\$ @πάνταῥεῖ Do you mean event sources as in members who call the submit() method ? Or another different, parallel event system ? \$\endgroup\$ – Niss36 Jan 2 '17 at 16:17
  • \$\begingroup\$ Well, I'd prefer a single Application object that is instantiated in the Main function, and passing interface (implementations) around with the components. That reduces coupling and dependencies in your application. Setting up the concrete couplings should be left to the Application, may be at an initialization phase in a run() function. Also, having a look at Factory Pattern_s may be helpful for an improved design. Factories often are,- but not necessarily need to be -, _Singletons. \$\endgroup\$ – πάντα ῥεῖ Jan 2 '17 at 16:49
1
\$\begingroup\$

Below are my remarks followed by the code including them.

Use accessors

It is best practice to use getter/setter to retrieve member of a class. As such, the members "source" and "eventType" of the class GameEvent should be private.

BUT: for a game where performance is crucial, I would say that using public final member is ok for data kind class like GameEvent.

I am not an expert on this point, but it might be possible that by declaring the GameEvent class final, the getter might be inlined by the JVM...

Use {} even where it is optional

Even though there are not required, I would strongly advice using {} in single if/while statement. So

if (list != null)
           list.forEach(listener -> listener.executeEvent(e));

Should be

if (list != null) {
   list.forEach(listener -> listener.executeEvent(e));
}

Use final keyword in method when possible

It increases readability. For instance in the dispatch method, the variable list could be final.

Use explicit name for variables and methods

Single character names are not very explicit (besides maybe for the exceptions in the catch clause). For instance 'e' for a GameEvent (should be 'event'), 'e' for a Logger method (should be 'error').

Try avoiding null as much as possible

Most of the time, you can avoid returning null value and then simplify your code. For instance:

 List<GameEventListener> list = typeListenersMap.get(type);

 if (list == null || !list.remove(listener))
     throw new IllegalArgumentException("Trying to unregister a non-registered listener");

Can be replaced by

    final List<GameEventListener> list = 
                    typeListenersMap.getOrDefault(type, Collections.emptyList());

     if (!list.remove(listener)) {
            throw new IllegalArgumentException("Trying to unregister a non-registered listener");
     }

Singleton or not Singleton

In your context, a singleton make sense, but this seems in contradiction with the motivation of "decreasing coupling between different parts of my game".

I would not advice for it as in the long term, singleton can be annoying.

If you go for the singleton, I would not make the dispatcher a singleton but instead I would use a single singleton for your application (like Application) and provide a method to retrieve the dispatcher:

public class Application {

    private static final Application INSTANCE = new Application();

    public static Application getInstance() {
        return INSTANCE;
    }

    private final GameEventDispatcher gameEventDispatcher;

    public Application() {
        //... initialize the game event dispatcher
    }

    public GameEventDispatcher getMainEventDispatcher() {
        return this.gameEventDispatcher;
    }

}

You then could add some other dispatchers in case you need it. For instance you might want the GUI to be updated as soon as possible then using a GUIEventDispatcher could be appropriate specially if the MainEventDispatcher is flooded by A.I. events.

'AWT-style' or not

I do not know what kind of game you are doing but if it is a complex one then I assume it will be multi-threaded (one thread for the I.A., one for the GUI ...). In that case, using a specific thread for the event has benefits:

  1. Dispatching an event will not block the thread dispatching it
  2. All listeners will receive the event in the same thread, so in a way, event handling is single threaded which can ease the logic

But it can complicate a bit things since there is no guarantee that is has been taken into account right after calling dispatch.

No using a specific thread will have the inverse in pro/cons.

Use BlockingQueue instead of Queue

In the dispatcher, you poll an event from the queue, if it is null you block until someone indicates that an event is available and then you re-poll the queue. Well BlockingQueue provides a method that does exactly that: BlockingQueue#take().

So in your case, I would use a LinkedBlockingQueue and remove all the code used to wait for an event.

Handle InterruptedException

See this answer. In your code, you just print its stack trace, but the exception has been thrown because someone asked the thread to stop and as such you should stop it.

Use Thread#interrupt() to interrupt a thread

Use the provided API to stop a thread instead of using its own one. This required to keep a reference to the Thread, but other methods need a reference to at least one boolean.

Below is a simple template:

    final Thread thread = new Thread(() -> {
        while (!Thread.currentThread().isInterrupted()) {
            try {
                //do some stuff
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
                break;
            } catch (Exception e) {
                //handle unexpected exception...
            }
        }
    });
    thread.start();

And to stop the thread:

    thread.interrupt();

Checking for null value

Checking for null value at the beginning of methods (like in submit) is a good thing. But try to be consistent for readability, use one of those and stick to it:

If statement

if (value == null) {
   throw new NullPointerException("value must not be null");
}

Objects API

//it also returns the provided value
Objects.requireNonNull(value, "value must not be null");

assert

assert value!=null:"value must not be null"

For assert, you need to add the option '-ea' to the java compiler. For a game, that could be a good thing: debug you game with assert enabled and when everything is fine, disable the assert to gain some performance (not sure this is significant though).

Listener Management

In the dispatcher, there is a lot of code to handle listeners (add/remove/get). For readability I would put all that in a ListenerManager and delegate all the calls to it.

Moreover, you lock on your listener map every time you dispatch an event which can degrade you performance. I understand that was done to avoid concurrency with the register and unregister methods.

But the dispatch method only read the map. One way to improve is to assume that:

  1. dispatching a event is crucial, it should be as quick as possible
  2. Adding/Removing a listener, is not that crucial, it is not that frequent and it is mainly done when the game is initializing.

Then :

  1. Remove the synchronization in the dispatching method
  2. In register/unregister, keep the synchronization but make the modification on a deep copy of the map before assign it.

Check the GameEventListenerManager

General remarks

These are matter of taste I guess.

  1. In GameEvent I would not use the name 'type' instead of 'eventType': event.getType() looks better than event.getEventType().
  2. I am not a big fan of inner class. I would have put the class GameEvent.Type into its own class/file GameEventType (add then you need one less character to reference it).
  3. Shortcut methods like submit(Object source, GameEvent.Type type) are handy till you change the GameEvent class: you will have to fix all these handy methods.
  4. When dispatching an event, if 1 listener fails then the following one will not get the event.
  5. Listeners are called in the order they were added which is not generally the convention (in Swing it is the inverse for instance).

Modified classes

Below, the modified code. I rename the logger method e to 'error'. I did not put all the assert though.

GameEventType

public enum GameEventType {
    GAME_START,
    GAME_EXIT,
    GAME_PAUSE,
    GAME_RESUME,
    GAME_RESET,
    GAME_DEATH,
    CONFIG_RESET,
    KEYBINDINGS_RESET
}

GameEvent

public class GameEvent {

    private final Object source;

    private final GameEventType type;

    public GameEvent(Object source, GameEventType type) {
        this.source = source;
        this.type = type;
    }

    public Object getSource() {
        return source;
    }

    public GameEventType getType() {
        return type;
    }
}

GameEventListenerManager

public class GameEventListenerManager {

    private final Lock lock = new ReentrantLock();

    private Map<GameEventType, List<GameEventListener>> listenerByEventType = new HashMap<>();

    /**
     * Register a listener for a given event type
     * @param eventType the type of the event the listener must be register to
     * @param listener the listener to register
     */
    public void register(GameEventType eventType, GameEventListener listener) {
        lock.lock();
        try {
            final Map<GameEventType, List<GameEventListener>> copy = duplicateAListenerMap(listenerByEventType);
            listenerByEventType = addListenerToAListenerMap(copy, eventType, listener);
        } finally {
            lock.unlock();
        }
    }

    /**
     * Remove a listener of a given event type
     * @param eventType the type of the event the listener is register to
     * @param listener the listener to unregister
     */
    public void unregister(GameEventType eventType, GameEventListener listener) {
        lock.lock();
        try {
            if (this.isListenerNotInCurrentListenerMap(eventType,listener)) {
                throw new IllegalArgumentException("Trying to unregister a non-registered listener");
            }
            final Map<GameEventType, List<GameEventListener>> copy = duplicateAListenerMap(listenerByEventType);
            listenerByEventType = removeListenerFromAListenerMap(copy, eventType, listener);
        } finally {
            lock.unlock();
        }
    }

    /**
     * @param eventType an event type
     * @return a stream of all listeners registered to the given event type
     */
    public Stream<GameEventListener> getEventListeners(GameEventType eventType) {
        return this.listenerByEventType.getOrDefault(eventType,Collections.emptyList()).stream();
    }


    /**
     * Check if a listener is not register for the provided event type in the current listener map
     * @return true if not the listener is not register for the given event type
     */
    private boolean isListenerNotInCurrentListenerMap(GameEventType type, GameEventListener listener) {
        return !listenerByEventType.getOrDefault(type, Collections.emptyList()).contains(listener);
    }

    /**
     * @return a deep copy of the provide map
     */
    private static Map<GameEventType, List<GameEventListener>> duplicateAListenerMap(Map<GameEventType, List<GameEventListener>> mapToDuplicate) {
        return mapToDuplicate.entrySet()
                .stream()
                .collect(Collectors.toMap(Map.Entry::getKey, e -> new ArrayList<>(e.getValue())));
    }

    /**
     * @return add a listener for a given event type to the provided map and returns it
     */
    private static Map<GameEventType, List<GameEventListener>> addListenerToAListenerMap(Map<GameEventType, List<GameEventListener>> mapToAddTo, GameEventType eventType, GameEventListener listener) {
        mapToAddTo.computeIfAbsent(eventType, t -> new ArrayList<>()).add(listener);
        return mapToAddTo;
    }

    /**
     * @return remove a listener for a given event type to the provided map and returns it
     */
    private static Map<GameEventType, List<GameEventListener>> removeListenerFromAListenerMap(Map<GameEventType, List<GameEventListener>> mapToRemoveFrom, GameEventType eventType, GameEventListener listener) {
        mapToRemoveFrom.computeIfPresent(eventType, (t,l) -> {
            l.remove(listener);
            return l.isEmpty()?null:l;
        });
        return mapToRemoveFrom;
    }
}

GameEventDispatcher

public class GameEventDispatcher {

    private static final Logger LOGGER = Logger.get(); //Internal logging utility

    private final GameEventListenerManager listenerManager = new GameEventListenerManager();

    private final BlockingQueue<GameEvent> eventsQueue = new LinkedBlockingQueue<>();

    private final Thread dispatchingThread;

    public GameEventDispatcher() {
        this.dispatchingThread = new Thread(this::dispatchingLoop, "GameEventDispatcher Thread");
        //set this thread as daemon. It will not prevent the application from exiting
        this.dispatchingThread.setDaemon(true);
        this.dispatchingThread.start();
    }


    public void interrupt() {
        this.dispatchingThread.interrupt();
    }

    public void register(GameEventType type, GameEventListener listener) {
        this.listenerManager.register(type,listener);
    }

    public void unregister(GameEventType type, GameEventListener listener) {
        this.listenerManager.unregister(type,listener);
    }

    public void submit(GameEvent event) {
        assert event !=null:"Event cannot be null";
        if (this.dispatchingThread.isInterrupted()) {
            LOGGER.warn("The dispatching thread has been interrupted. Submission is ignored for event '"+event+"'");
        }
        eventsQueue.add(event);
    }

    private void dispatch(GameEventListener listener, GameEvent event) {
        try {
            listener.executeEvent(event);
        } catch (Exception e) {
            //listener should handle its exception
            //this is probably a bug.
            //Log the error for the moment
            LOGGER.error("Exception occurred while dispatching event'"+event+"' to listener '"+listener+"'",e);
        }
    }

    private void dispatchingLoop() {
        while (!Thread.currentThread().isInterrupted()) {
            try {
                final GameEvent event = eventsQueue.take();
                listenerManager.getEventListeners(event.getType()).forEach(l -> dispatch(l,event));
            } catch (InterruptedException e) {
                //someone asked to stop this thread
                //set back the interrupt flag and
                //quit the loop
                Thread.currentThread().interrupt();
                break;
            }
        }
    }

}
\$\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.