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

My EventBus system followup

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

Problem

This question is a followup from my previous question My EventBus system, and incorporates most points from @rolfl's answer.

It includes, but is not limited to:

  • Usage of Collections.synchronizedSet over manual synchronized { } on trivial methods.



  • Minimal locking



  • High performance code, but not going into micro-optimizations if it harms the readability of the code.



The code:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface Event { }


public interface EventBus {
    void registerListenersOfObject(final Object callbackObject);

     void registerListener(final Class eventClass, final Consumer eventListener);

    void executeEvent(final Object event);

    void removeListenersOfObject(final Object callbackObject);

     void removeListener(final Class eventClass, final Consumer eventListener);

    void removeAllListenersOfEvent(final Class eventClass);

    void removeAllListeners();
}


```
public class SimpleEventBus implements EventBus {
private final static Set EMPTY_SET = new HashSet<>();
private final static EventHandler[] EMPTY_ARRAY = new EventHandler[0];

private final ConcurrentMap, Set> eventMapping = new ConcurrentHashMap<>();
private final Class eventClassConstraint;

public SimpleEventBus() {
this(Object.class);
}

public SimpleEventBus(final Class eventClassConstraint) {
this.eventClassConstraint = Objects.requireNonNull(eventClassConstraint);
}

@Override
public void registerListenersOfObject(final Object callbackObject) {
Arrays.stream(callbackObject.getClass().getMethods())
.filter(this::isEligibleMethod)
.forEach(method -> {
Class eventClass = method.getParameterTypes()[0];
addEventHandler(eventClass, new MethodEventHandler(method, callbackObject, eventClass));
});
}

@Override
@SuppressWarnings("unchecked")
public void r

Solution

There are two bugs related to type system:

Currently if all three parameter types, registerListener's first, and registered Consumer's and executeEvent's, it works as expected.

@Test
public void worksAsExpected() {
    String expected = "WORKS";
    AtomicReference actual = new AtomicReference<>(null);
    EventBus bus = new SimpleEventBus(String.class);
    bus.registerListener(String.class, (String s)-> {actual.set(s);});
    bus.executeEvent(expected);
    assertEquals(expected, actual.get());
}


In the below case I register a listener for events of type Number and execute and event of type Number but it fails to execute the handler:

@Test
public void shouldHaveWorkedButDoesNot() {
    Number expected = 1;
    AtomicReference actual = new AtomicReference<>(null);
    EventBus bus = new SimpleEventBus(Number.class);
    bus.registerListener(Number.class, (Number n)-> {actual.set(n);});
    bus.executeEvent(expected);
    assertEquals(expected, actual.get());
}


This happens because in the getCopyOfEventHandlers only gets the handlers registered for runtime type of the executed event. It should get handlers registered for the runtime type or any super-type thereof.

To fix this; getCopyOfEventHandlers should be something like:

private Set getEventHandlersFor(final Class eventClass) {
    return eventMapping.entrySet().stream()
            .filter(entry -> entry.getKey().isAssignableFrom(eventClass))
            .flatMap(entry -> entry.getValue().stream())
            .collect(Collectors.toSet());
}


The second type problem is this:

When I register a handler for Numbers and it will run for any Numbers. Some might be Floats, others might be AtomicLongs. If I register a Consumer to handle Numbers and pass in a Float, which is a Number, I will get a runtime exception.

@Test
public void shouldNotCompileButDoes() {
    EventBus bus = new SimpleEventBus(Number.class);
    // Not all Numbers are AtomicIntegers so this is a type error
    bus.registerListener(Number.class, 
            (AtomicInteger n)-> {System.out.println(n.get());});
    Number someNumber = 4f;
    bus.executeEvent(someNumber); // e.g. Float does not have .get()
}


Conversely if I register a Consumer to handle Numbers and pass in any Float, I will always run. In fact obviously a Consumer can accept any object.

@Test
public void shouldCompileButDoesNot() {
    EventBus bus = new SimpleEventBus(Number.class);
    // All Numbers are Objects this would work
//    bus.registerListener(Number.class, 
//            (Object o)-> {System.out.println(o);});
    Number someNumber = 4f;
    bus.executeEvent(someNumber);
}


To fix this; you should replace Consumer eventListener in registerListeners signature with Consumer. In fact because the T only appears a parameter type in Consumer declaration, a parameter of type Consumer should either be a type error or could be replaced by just Consumer. I suspect latter might be the case for removeListener.

Code Snippets

@Test
public void worksAsExpected() {
    String expected = "WORKS";
    AtomicReference<String> actual = new AtomicReference<>(null);
    EventBus bus = new SimpleEventBus(String.class);
    bus.registerListener(String.class, (String s)-> {actual.set(s);});
    bus.executeEvent(expected);
    assertEquals(expected, actual.get());
}
@Test
public void shouldHaveWorkedButDoesNot() {
    Number expected = 1;
    AtomicReference<Number> actual = new AtomicReference<>(null);
    EventBus bus = new SimpleEventBus(Number.class);
    bus.registerListener(Number.class, (Number n)-> {actual.set(n);});
    bus.executeEvent(expected);
    assertEquals(expected, actual.get());
}
private Set<EventHandler> getEventHandlersFor(final Class<?> eventClass) {
    return eventMapping.entrySet().stream()
            .filter(entry -> entry.getKey().isAssignableFrom(eventClass))
            .flatMap(entry -> entry.getValue().stream())
            .collect(Collectors.toSet());
}
@Test
public void shouldNotCompileButDoes() {
    EventBus bus = new SimpleEventBus(Number.class);
    // Not all Numbers are AtomicIntegers so this is a type error
    bus.registerListener(Number.class, 
            (AtomicInteger n)-> {System.out.println(n.get());});
    Number someNumber = 4f;
    bus.executeEvent(someNumber); // e.g. Float does not have .get()
}
@Test
public void shouldCompileButDoesNot() {
    EventBus bus = new SimpleEventBus(Number.class);
    // All Numbers are Objects this would work
//    bus.registerListener(Number.class, 
//            (Object o)-> {System.out.println(o);});
    Number someNumber = 4f;
    bus.executeEvent(someNumber);
}

Context

StackExchange Code Review Q#48970, answer score: 5

Revisions (0)

No revisions yet.