6

I have several types of events, for example:

abstract class Event {
    static class KeyPress extends Event { ... }
    static class KeyRelease extends Event { ... }
    static class KeyHold extends Event { ... }
    // ...
}

And many listeners which respond to some of the above events by registering them in an event handler. It looks like this currently:

abstract class AbstractListener {
    Set<Class<? extends Event>> eventTypes;
    protected abstract boolean respond(Event event);
}

class DownKeyListener exteneds AbstractListener {
    DownKeyListener () {
        // just prepares to register to receive these events. doesn't matter how exactly.
        eventTypes.add(KeyPress.class);
        eventTypes.add(KeyHold.class);
        //no KeyRelease e.g.
    }

    boolean respond(Event event) {
        if (event instanceof KeyPress)
            return handleKeyPress(event);
        else if (event instanceof KeyHold)
            return handleKeyHold(event);
        return false;
    }

    private boolean handleKeyPress(KeyPress e) { ... }
    private boolean handleKeyHold(KeyHold e) { ... }
}

What I don't like about this is that there is nothing forcing or checking at least the relation between the registered events and the checks for them and handling in the respond method. This keeps leading to developer bugs. it's also a lot of code with instanceofs for little benefit (I would say).

So I though about doing something "smart" like this: create a map between the event types and the handlers so each event that is registered for handling will have a handler:

abstract class AbstractListener {
    Map<Class<? extends Event>, Function<? extends Event, Boolean> map = new ...
    //             ^ doesn't ensure these event ^: are the same but at least
    //                                             that someone responds
    // to ensure same event i can do
    protected <T extends Event> void register(Class<T> event, Function<T, Boolean> function) {
        map.put(event, function);
    }

    protected abstract boolean respond(Event event);
}

And then:

class DownKeyListener exteneds AbstractListener {
    DownKeyListener () {
        Map.put(KeyPress.class, keyPressFunction);
        Map.put(KeyHold.class, keyHoldFunction);
    }

    boolean respond(Event event) {
        Function<? extends Event, Boolean> f = map.get(event.getClass());
        return f == null ? false : f.apply(event);
    }

    Function<KeyPress, Boolean> keyPressFunction = event -> ...;
    Function<KeyHold, Boolean> keyHoldFunction = event -> ...;
}

Of course this doesn't work because of generics. apply gives an error and I understand why

The method apply(capture#4-of ? extends Event) in the type Function<capture#4-of ? extends Event,Boolean> is not applicable for the arguments (capture#5-of ? extends Event)

I don't know how to get what I want working correctly. Some things I had in mind:

  • Cast the result of map.get to something that will ensure it works properly
  • Change the respond method to be generic protected abstract <T extends Event> boolean respond(T event); which give the similar error:
The method apply(capture#3-of ? extends Event) in the type Function<capture#3-of ? extends Event,Boolean> is not applicable for the arguments (T)

Anyone has a suggestion on achieving what I want in any way?

  • From an OO perspective this looks like utter nonsense. Which is likely why no one has responded yet, they just do not understand what you are on about. I see an attempt at a general solution for handling events but no context whatsoever. No class to feature the events, no subscriptions. You may want to simplify your example into something that works and then provide another example that starts solving your problem. – Martin Maat Feb 26 '17 at 15:52
  • @MartinMaat what kind of context are you missing? "No class to feature the events" what? the Event class features the events. "no subscriptions" subscription is done after the event types are added in the constructor. How they are sent to the event manager isn't important unless you want more code but then you ask to simplify it. I don't understand what you want. – Mark Feb 26 '17 at 16:03
  • @Mark "The event class features the events" You may be thinking of a different type of event than any OO trained developer would when he hears the term. If this is the case your question would need some substantial introduction. – Martin Maat Feb 26 '17 at 16:40
  • The Event class has what look like inner classes, but they don't use the class keyword. This isn't Java code, it's some weird pseudo-code. – David Conrad Feb 27 '17 at 14:40
  • @DavidConrad i fixed it but honestly it was quite obvious. all the answers treated them as classes and even copy pasted with this mistake. it's not a weird pseudo-code if only 1 place is missing an obvious keyword. – Mark Mar 2 '17 at 3:02
0

You might consider making AbstractListener be parameterized by the concrete type of Event that you want to handle.

abstract class AbstractListener<T extends Event> {
    ...
    protected abstract boolean respond(T event);
}

Then have:

class KeyPressListener extends AbstractListener<KeyPressEvent> {
    ....
}

You'd need separate concrete handlers, but you could have them both dispatch to something that could handle processing from different types of events.

  • But each listener can handle any number of events. Java generics don't support this runtime resolution. Or am I msising your point? – Mark Feb 27 '17 at 7:16
  • My suggestion is that if you want to enforce that an X listener handles X event, then write the code to enforce that. If the listeners for X and Y events have something in common, factor that common code and let the X and Y listeners delegate to the common code. – Andy Dalton Feb 27 '17 at 18:34
  • But then how can the listener handle more than 1 event? Can you show in code like I did please? – Mark Mar 1 '17 at 17:30
2

You could use a visitor pattern, like so:

abstract class Event {
    boolean accept(EventVisitor visitor);

    KeyPress extends Event {
        boolean accept(EventVisitor visitor)
        {
            return visitor.process(this);
        }
    }
    KeyRelease extends Event { ... }
    KeyHold extends Event { ... }
    // ...
}

abstract class EventVisitor
{
    boolean process(KeyPress event)
    {
        return false;
    }

    boolean process(KeyRelease event)
    {
        return false;
    }

    // One default process() method for each subclass of Event.

    boolean respond(Event event)
    {
        return event.accept(this);
    }
}

and then

class DownKeyListener extends EventVisitor
{
    void process(KeyPress event)
    {
        return handleKeyPress(event);
    }

    void process(KeyHold event)
    {
        return handleKeyHold(event);
    }
}

This solution ensures that

  1. You avoid all the instanceof needed to dispatch on the event's type.
  2. All events that are not explicitly handled by a visitor implementation have a default handling in the abstract class EventVisitor.
  • Thanks. this is indeed an idea (can't upvote yet, sorry). It looks to me like all events have the same accept method. So why not put it in the superclass? – Mark Feb 27 '17 at 7:14
  • @Mark it looks the same only because the type of this is not depicted in visitor.process(this), the call is bound to a different overload in each case. The visitor pattern is needed in Java because it only does virtual dispatch on one parameter. – Caleth Feb 27 '17 at 14:09
  • So when I create a new event I need to remember to add a process method for it in the visitor and i need to remember to register to that event in the listener? – Mark Mar 1 '17 at 18:02
  • Regarding the visitor, the compiler will remind you as soon as you implement the accept method of the new event class. – Giorgio Mar 1 '17 at 18:13
0

Another options is to stop "fighting" the limited type system. My understanding is you want to ensure that anything you want to listen to has a handler, and that the right handler gets called. Then those two steps should be combined. You haven't shown what exactly you are listening to, but I am guessing you have something observable.

observable.register<KeyPress>(new EventListener() {
    void handleEvent(Event e) {
        assert e instanceof KeyPress;
        KeyPress kpe = (KeyPress)e;
        //Do stuff with kpe
    }
});

This does involve a cast, but you can use assert to check during development, and the cast is right next to the registration. Of course you do not have to use an anonymous class:

observable.register<KeyPress>(new MyKeyPressHandler());

You can modify the return type to boolean if you need to.


An example of how it might work:

// Could also be abstract class
interface Event {
}

interface EventListener {
    void handleEvent(Event e);
}

interface Observable {
    public void register(Class<? extends Event> event, EventListener handler);
}

// An example of something observable - here how a keyboard might give events.
class KeyDownEvent {
    private Key key;
    KeyDownEvent(Key k) {
        key = k;
    }
    public Key getKey() {
        return key;
    }
}

class Keyboard implements Observable {
    private Map<Class<? extends Event>, Set<EventListener>> handlers = new HashMap<>();

    public void register(Class<? extends Event> event, EventListener handler) {
        // not handling if the set has not been defined yet, but it should be, (or in constructor).
        handlers.put(event, handler);
    }

    private notifyListeners(Event event) {
        Class eventClass = event.getClass();
        for (EventListener listener : handlers.get(eventClass)) {
            listener.handleEvent(event);
        }
    }

    private void updateKeyboardState(KeyboardState ks) {
        //Suppose this is called by some code to update the keyboard
        // get state
        for (Key k : keys) {
            if (ks.isPressed(k) && lastState.get(k) == KeyState.UP) {
                notifyListeners(new KeyDownEvent(k));
            }
        }
    }
}

class LogKeyPressesToConsole {
    LogKeyPressesToConsole(Keyboard keyboard) {
        keyboard.register(KeyDownEvent, new EventListener() {
            void handleEvent(Event event) {
                assert event instanceof KeyDownEvent;
                KeyDownEvent kde = (KeyDownEvent)event;
                System.out.printf(kde.getKey().toString());
            }
        });
    }
}
  • I don't understand how this would work for multiple types and what exactly is your observable. Can you give an example like I did? – Mark Mar 1 '17 at 19:19
  • The observable is whatever object generates the events. It was not clear from you post where exactly the events come from. Not sure what you about multiple types? I have added an example showing how it might fit together. – senevoldsen Mar 3 '17 at 23:07
0

I figured it out using my generics approach. My second point about changing respond to a generic <T extends Event> boolean respond(T event) was indeed the right approach. The compiler error for incompatible types can be solved with a dirty but safe cast. Here is the full working code

public class Test {

    public static void main(String[] args) {
        DownKeyListener dkl = new DownKeyListener();
        System.out.println(dkl.respond(new KeyPress()));
        System.out.println(dkl.respond(new KeyHold()));
        System.out.println(dkl.respond(new KeyRelease()));
    }
}

abstract class Event {
    static class KeyPress extends Event {}
    static class KeyHold extends Event {}
    static class KeyRelease extends Event {}
}

abstract class AbstractListener {

    private Map<Class<? extends Event>, Function<? extends Event, Boolean>> map = new HashMap<>();

    protected <T extends Event> void register(Class<T> event, Function<T, Boolean> function) {
        map.put(event, function);
    }

    <T extends Event> boolean respond(T event) {
        Function<? extends Event, Boolean> f = map.get(event.getClass());
        return f == null ? false : ((Function<T, Boolean>) f).apply(event);
    }
}

class DownKeyListener extends AbstractListener {

    DownKeyListener() {
        register(KeyPress.class, keyPressHandler);
        register(KeyHold.class, keyHoldHandler);
    }

    Function<KeyPress, Boolean> keyPressHandler = e -> true;
    Function<KeyHold, Boolean> keyHoldHandler = e -> true;
}

The output of running this is

true
true
false

as required.

There are 3 things i want to point out about this approach.

The first is the ease of use and compile time safety. It couples the event registering (the list of event to listen to which in this cases is the map's keys) and the event handling. For every event type which is registered a handler for that event type must be given and when the listener needs to respond to an event that same handler is used. Creating a new event type does not entail any additional work like visitor methods or new interfaces. It is created and if some listener wants to use it it's ready.

The second thing is that the respond method can be moved to the abstract superclass because it has the same behavior for all listeners: get the handler for the event, call it and return the result. If a subclass listener wants to change this for a specific event type they can override it, check the event type and for the rest call super.

The third thing is that cast and the loss of information between putting the event-function in the map and getting the function from the event. The map is declared with 2 wildcards with no way to force a relation between them as far as i know. this is because of the lack of generic fields in java. if such a thing ever came to exist it would simplify this approach. The forcing of the same types for the event and its handler is done in the register method which adds to the map but that enforcement is lost in the field declaration. That means that when getting the value from the key there is no more compile time correspondence and a cast has to be made to the same type. We know this cast is safe if we only used the register method to populate the map otherwise we're doomed. a generic field would eliminate the need for a "safe" cast.

That's it. I learned a lot from the different answers and my fighting with Java generics and I post here verbosely for the benefit of everyone else. Hopefully this helps someone else in the future.

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.