Simple Event Dispatcher for Game

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 eventsQueue = new ConcurrentLinkedQueue<>();
private final Map> 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();
}

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 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 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:

• Dispatching an event will not block the thread dispatching it

• 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 beginn

StackExchange Code Review Q#151473, answer score: 2