refactor so that any java 8 or later can use lambdas for Activate and… #175
Conversation
… Track listeners.
Pull Request Test Coverage Report for Build 488
💛 - Coveralls |
Pull Request Test Coverage Report for Build 495
💛 - Coveralls |
| @@ -16,9 +16,13 @@ | |||
| */ | |||
| package com.optimizely.ab.notification; | |||
|
|
|||
| import com.optimizely.ab.config.Experiment; | |||
There was a problem hiding this comment.
Can you change the license header to say 2017-2018. Let's make sure to do this for the files we touch in the PR
| /** | ||
| * Convenience method to support lambdas as callbacks in later version of Java (8+). | ||
| * @param activateNotificationListenerInterface | ||
| * @return greater than zero if added. |
There was a problem hiding this comment.
I wonder if we will run into concurrency/threading issues with incrementing this static variable. What if we end up with the same ID for different listeners?
There was a problem hiding this comment.
not static. you are looking at the holder class def and not the notification id.
| @Nonnull Map<String, String> attributes, | ||
| @Nonnull Variation variation) { | ||
| } | ||
| public interface NotificationListener { | ||
|
|
||
| /** | ||
| * This is the new method of notification. Implementation classes such as {@link ActivateNotificationListener} |
There was a problem hiding this comment.
just a nitpick: No need to say this method is new. I think we can handle that in the changelogs
|
|
||
| /** | ||
| * This is the new method of notification. Implementation classes such as {@link ActivateNotificationListener} | ||
| * will implement this call and provide another method with the correct parameters | ||
| * Notify called when a notification is triggered via the {@link com.optimizely.ab.notification.NotificationCenter} | ||
| * @param args - variable argument list based on the type of notification. | ||
| */ | ||
| public abstract void notify(Object... args); | ||
| public void notify(Object... args); |
There was a problem hiding this comment.
instead of having variables args and attempting to parse them in the implementation, why not have a static notification object? I think that would be way easier to read in code
There was a problem hiding this comment.
You mean like javascript? We only did that for javascript since it is json. The advantage to using a vararg is that you can support any kind of notify (meaning you could use it for any notification ini the system). I don't know if that is a strong enough argument. The other thing is that all other SDKs use varargs.
|
can you try this out in a testapp to verify? |
|
build |
… Track listeners.