Type safe Event Bus #9

Merged
merged 3 commits into from Jan 27, 2016

Projects

None yet

5 participants

@hendrikebbers
Member

The event bus is type safe (by generics)
ToDo Example uses event Bus and syncs the state of ToDos

Review on Reviewable

@hendrikebbers hendrikebbers Type safe Event Bus
ToDo Example uses event Bus and syncs the state of ToDos
9dd4581
@fadre
fadre commented Dec 16, 2015

Definitely an improvement over the version that only returns Object in my opinion, although slightly more difficult to call publish and subscribe (a Topic must be created).

@hendrikebbers
Member

@fdreier mmh, normally you would create a topic as a static String an reuse it:

private final static String ADD_TOPIC = "add_item";

With the new version you create a Topic instance mainly the same way:

private final static Topic<DataType> ADD_TOPIC = Topic.create("add_item");

Don't think that this is more complex :)

@Dierk
Contributor
Dierk commented Dec 16, 2015

Hm - this change is about making it more type safe?

@fadre
fadre commented Dec 16, 2015

@hendrikebbers , or you could write
private final static Topic ADD_TOPIC = new Topic("add_item");

so there are two possibilities -> therefor SLIGHTLY more complex ;)

@hendrikebbers
Member

@Dierk @fdreier Oh, Markdown removed some chars in the code block. Now it's formatted as code and the generics are visible:

private final static Topic<DataType> ADD_TOPIC = Topic.create("add_item");

By using the Topic you can only send data of type DataType to the topic. In addition you can only register listeners for the given type (DataType). This gives you type safety at development time. If you only use Strings or a Topic without generics you need to cast all data to the specific type in the listeners:

eventBus.subscripe(NOT_GENERIC_TOPIC, data -> ((DataType) data).someMethod());

I think this makes the code more complex / harder to read. In addition you can't send a different type when using generics since the code won't compile:

eventBus.publish(GENERIC_TOPIC, dataOfWrongType); <-- This won't compile
@hendrikebbers
Member

@fdreier The two calls

Topic<Data> t = new Topic<>("name");

and

Topic<Data> t = Topic.create("name");

does exactly the same. We can remove the static create-methods if this creates complexity. Normally I like the second option more when creating static fields like shown in the example. But that's only my opinion / feeling :)

For sure you still could do

private final static Topic ADD_TOPIC = new Topic("add_item");

that creates a topic that don't help you when talking about type safety. But this is the same as with collections. You still can create a List instance without using generics but it mostly don't make much sense.

@aalmiray
Contributor

It looks good to me

@netopyr
Collaborator
netopyr commented Jan 21, 2016

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.


java-server/src/main/java/com/canoo/dolphin/server/event/DolphinEventBus.java, line 46 [r1] (raw file):
This should be MessageListener<? super T>


java-server/src/main/java/com/canoo/dolphin/server/event/impl/DolphinEventBusImpl.java, line 57 [r1] (raw file):
If I remember correctly, "mustn't" should not be used in writing.


java-server/src/main/java/com/canoo/dolphin/server/event/impl/Receiver.java, line 71 [r1] (raw file):
Why are Message and MessageListener not typed?


java-server/src/main/java/com/canoo/dolphin/server/event/Topic.java, line 7 [r1] (raw file):
Should be final


java-server/src/main/java/com/canoo/dolphin/server/event/Topic.java, line 32 [r1] (raw file):
I think we should remove the double negation


java-server/src/test/java/com/canoo/dolphin/server/event/impl/TestDolphinEventBusImpl.java, line 91 [r1] (raw file):
This needs to be corrected. We cannot rely on Thread.sleep() in our tests.


Comments from the review on Reviewable.io

@aalmiray
Contributor

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.


java-server/src/test/java/com/canoo/dolphin/server/event/impl/TestDolphinEventBusImpl.java, line 91 [r1] (raw file):
I suggest having a look at https://github.com/jayway/awaitility


Comments from the review on Reviewable.io

@aalmiray
Contributor

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.


java-server/src/main/java/com/canoo/dolphin/server/event/impl/Receiver.java, line 71 [r1] (raw file):
can't type them because the listenersPerTopic map can handle multiple Topics with different type information


Comments from the review on Reviewable.io

@netopyr
Collaborator
netopyr commented Jan 22, 2016

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.


java-server/src/main/java/com/canoo/dolphin/server/event/impl/Receiver.java, line 71 [r1] (raw file):
In that case one could still use wildcards. Mixing raw and typed collections is usually a code smell. Either one has not tried hard enough ;) or something in the design is really broken.


Comments from the review on Reviewable.io

@aalmiray
Contributor

Review status: 0 of 11 files reviewed at latest revision, 6 unresolved discussions.


java-server/src/main/java/com/canoo/dolphin/server/event/impl/Receiver.java, line 71 [r1] (raw file):
true. Which wildcard setup should be used here other than simply <?> (which gains you nothing IMHO).


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers added this to the 0.8.0 milestone Jan 22, 2016
@hendrikebbers hendrikebbers based on review
fdeec00
@hendrikebbers
Member

I think this can be merged

@netopyr
Collaborator
netopyr commented Jan 26, 2016

Reviewed 7 of 11 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


Comments from the review on Reviewable.io

@netopyr
Collaborator
netopyr commented Jan 26, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


java-server/src/main/java/com/canoo/dolphin/server/event/impl/DolphinEventBusImpl.java, line 57 [r1] (raw file):
Well, not really ;)


Comments from the review on Reviewable.io

@hendrikebbers hendrikebbers mustn't -> must not
9911b2b
@hendrikebbers
Member

Review status: 10 of 11 files reviewed at latest revision, 2 unresolved discussions.


java-server/src/main/java/com/canoo/dolphin/server/event/impl/DolphinEventBusImpl.java, line 57 [r1] (raw file):
:P


Comments from the review on Reviewable.io

@hendrikebbers
Member

Review status: 10 of 11 files reviewed at latest revision, 2 unresolved discussions.


java-server/src/main/java/com/canoo/dolphin/server/event/impl/DolphinEventBusImpl.java, line 57 [r1] (raw file):
Done.


Comments from the review on Reviewable.io

@netopyr
Collaborator
netopyr commented Jan 27, 2016

Review status: 10 of 11 files reviewed at latest revision, 2 unresolved discussions.


java-server/src/test/java/com/canoo/dolphin/server/event/impl/TestDolphinEventBusImpl.java, line 91 [r1] (raw file):
Interesting library. I like in particular the integration with Hamcrest. Never felt the need for such a library, but I am also not against it.


Comments from the review on Reviewable.io

@netopyr
Collaborator
netopyr commented Jan 27, 2016

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from the review on Reviewable.io

@netopyr netopyr merged commit b10e953 into master Jan 27, 2016

3 of 4 checks passed

code-review/reviewable Review in progress: all files reviewed, 2 unresolved discussions
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.2%) to 15.197%
Details
@hendrikebbers hendrikebbers deleted the new-event-bus branch Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment