Skip to content

refactor so that any java 8 or later can use lambdas for Activate and… #175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 30, 2018
Merged

refactor so that any java 8 or later can use lambdas for Activate and… #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9cc418
refactor so that any java 8 or later can use lambdas for Activate and…
thomaszurkan-optimizely Mar 22, 2018
db8f932
make some updates according to mike's comments
thomaszurkan-optimizely Mar 29, 2018
a7718ae
add some unit tests to up code coverage
thomaszurkan-optimizely Mar 30, 2018
d6e9881
Merge branch 'master' into refactor/notification/forLambda
thomaszurkan-optimizely Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 0 additions & 39 deletions core-api/src/main/java/com/optimizely/ab/Optimizely.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import com.optimizely.ab.event.internal.EventBuilder;
import com.optimizely.ab.event.internal.payload.EventBatch.ClientEngine;
import com.optimizely.ab.internal.EventTagUtils;
import com.optimizely.ab.notification.NotificationBroadcaster;
import com.optimizely.ab.notification.NotificationCenter;
import com.optimizely.ab.notification.NotificationListener;
import org.slf4j.Logger;
Expand Down Expand Up @@ -90,7 +89,6 @@ public class Optimizely {
@VisibleForTesting final ProjectConfig projectConfig;
@VisibleForTesting final EventHandler eventHandler;
@VisibleForTesting final ErrorHandler errorHandler;
@VisibleForTesting final NotificationBroadcaster notificationBroadcaster = new NotificationBroadcaster();
public final NotificationCenter notificationCenter = new NotificationCenter();

@Nullable private final UserProfileService userProfileService;
Expand Down Expand Up @@ -205,8 +203,6 @@ private void sendImpression(@Nonnull ProjectConfig projectConfig,
logger.error("Unexpected exception in event dispatcher", e);
}

notificationBroadcaster.broadcastExperimentActivated(experiment, userId, filteredAttributes, variation);

notificationCenter.sendNotifications(NotificationCenter.NotificationType.Activate, experiment, userId,
filteredAttributes, variation, impressionEvent);
} else {
Expand Down Expand Up @@ -245,12 +241,9 @@ public void track(@Nonnull String eventName,
// attributes.
Map<String, String> filteredAttributes = filterAttributes(currentConfig, attributes);

Long eventValue = null;
if (eventTags == null) {
logger.warn("Event tags is null when non-null was expected. Defaulting to an empty event tags map.");
eventTags = Collections.<String, String>emptyMap();
} else {
eventValue = EventTagUtils.getRevenueValue(eventTags);
}

List<Experiment> experimentsForEvent = projectConfig.getExperimentsForEventKey(eventName);
Expand Down Expand Up @@ -293,8 +286,6 @@ public void track(@Nonnull String eventName,
logger.error("Unexpected exception in event dispatcher", e);
}

notificationBroadcaster.broadcastEventTracked(eventName, userId, filteredAttributes, eventValue,
conversionEvent);
notificationCenter.sendNotifications(NotificationCenter.NotificationType.Track, eventName, userId,
filteredAttributes, eventTags, conversionEvent);
}
Expand Down Expand Up @@ -729,36 +720,6 @@ public UserProfileService getUserProfileService() {
return userProfileService;
}

//======== Notification listeners ========//

/**
* Add a {@link NotificationListener} if it does not exist already.
*
* @param listener listener to add
*/
@Deprecated
public void addNotificationListener(@Nonnull NotificationListener listener) {
notificationBroadcaster.addListener(listener);
}

/**
* Remove a {@link NotificationListener} if it exists.
*
* @param listener listener to remove
*/
@Deprecated
public void removeNotificationListener(@Nonnull NotificationListener listener) {
notificationBroadcaster.removeListener(listener);
}

/**
* Remove all {@link NotificationListener}.
*/
@Deprecated
public void clearNotificationListeners() {
notificationBroadcaster.clearListeners();
}

//======== Helper methods ========//

/**
Expand Down
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/notification/ActivateNotificationListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import java.util.Map;


public abstract class ActivateNotificationListener extends NotificationListener {
public abstract class ActivateNotificationListener implements NotificationListener, ActivateNotificationListenerInterface {

/**
* Base notify called with var args. This method parses the parameters and calls the abstract method.
Expand Down
25 changes: 25 additions & 0 deletions ...i/src/main/java/com/optimizely/ab/notification/ActivateNotificationListenerInterface.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.optimizely.ab.notification;

import com.optimizely.ab.config.Experiment;
import com.optimizely.ab.config.Variation;
import com.optimizely.ab.event.LogEvent;

import javax.annotation.Nonnull;
import java.util.Map;

public interface ActivateNotificationListenerInterface {
/**
* onActivate called when an activate was triggered
* @param experiment - The experiment object being activated.
* @param userId - The userId passed into activate.
* @param attributes - The filtered attribute list passed into activate
* @param variation - The variation that was returned from activate.
* @param event - The impression event that was triggered.
*/
public void onActivate(@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, String> attributes,
@Nonnull Variation variation,
@Nonnull LogEvent event) ;

}
117 changes: 0 additions & 117 deletions core-api/src/main/java/com/optimizely/ab/notification/NotificationBroadcaster.java
View file
Open in desktop

This file was deleted.

55 changes: 48 additions & 7 deletions core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
*
* Copyright 2017, Optimizely and contributors
* Copyright 2017-2018, Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,9 +16,13 @@
*/
package com.optimizely.ab.notification;

import com.optimizely.ab.config.Experiment;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import com.optimizely.ab.config.Variation;
import com.optimizely.ab.event.LogEvent;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.Nonnull;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -78,6 +82,43 @@ public NotificationCenter() {
// we used a list so that notification order can mean something.
private Map<NotificationType, ArrayList<NotificationHolder>> notificationsListeners =new HashMap<NotificationType, ArrayList<NotificationHolder>>();

/**
* Convenience method to support lambdas as callbacks in later version of Java (8+).
* @param activateNotificationListenerInterface
* @return greater than zero if added.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not static. you are looking at the holder class def and not the notification id.

*/
public int addActivateNotificationListener(final ActivateNotificationListenerInterface activateNotificationListenerInterface) {
if (activateNotificationListenerInterface instanceof ActivateNotificationListener) {
return addNotificationListener(NotificationType.Activate, (NotificationListener)activateNotificationListenerInterface);
}
else {
return addNotificationListener(NotificationType.Activate, new ActivateNotificationListener() {
@Override
public void onActivate(@Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map<String, String> attributes, @Nonnull Variation variation, @Nonnull LogEvent event) {
activateNotificationListenerInterface.onActivate(experiment, userId, attributes, variation, event);
}
});
}
}

/**
* Convenience method to support lambdas as callbacks in later versions of Java (8+)
* @param trackNotificationListenerInterface
* @return greater than zero if added.
*/
public int addTrackNotificationListener(final TrackNotificationListenerInterface trackNotificationListenerInterface) {
if (trackNotificationListenerInterface instanceof TrackNotificationListener) {
return addNotificationListener(NotificationType.Activate, (NotificationListener)trackNotificationListenerInterface);
}
else {
return addNotificationListener(NotificationType.Track, new TrackNotificationListener() {
@Override
public void onTrack(@Nonnull String eventKey, @Nonnull String userId, @Nonnull Map<String, String> attributes, @Nonnull Map<String, ?> eventTags, @Nonnull LogEvent event) {
trackNotificationListenerInterface.onTrack(eventKey, userId, attributes, eventTags, event);
}
});
}
}

/**
* Add a notification listener to the notification center.
Expand All @@ -86,7 +127,7 @@ public NotificationCenter() {
* @param notificationListener - Notification to add.
* @return the notification id used to remove the notification. It is greater than 0 on success.
*/
public int addNotification(NotificationType notificationType, NotificationListener notificationListener) {
public int addNotificationListener(NotificationType notificationType, NotificationListener notificationListener) {

Class clazz = notificationType.notificationTypeClass;
if (clazz == null || !clazz.isInstance(notificationListener)) {
Expand All @@ -107,11 +148,11 @@ public int addNotification(NotificationType notificationType, NotificationListen
}

/**
* Remove the notification listener based on the notificationId passed back from addNotification.
* Remove the notification listener based on the notificationId passed back from addNotificationListener.
* @param notificationID the id passed back from add notification.
* @return true if removed otherwise false (if the notification is already registered, it returns false).
*/
public boolean removeNotification(int notificationID) {
public boolean removeNotificationListener(int notificationID) {
for (NotificationType type : NotificationType.values()) {
for (NotificationHolder holder : notificationsListeners.get(type)) {
if (holder.notificationId == notificationID) {
Expand All @@ -130,17 +171,17 @@ public boolean removeNotification(int notificationID) {
/**
* Clear out all the notification listeners.
*/
public void clearAllNotifications() {
public void clearAllNotificationListeners() {
for (NotificationType type : NotificationType.values()) {
clearNotifications(type);
clearNotificationListeners(type);
}
}

/**
* Clear notification listeners by notification type.
* @param notificationType type of notificationsListeners to remove.
*/
public void clearNotifications(NotificationType notificationType) {
public void clearNotificationListeners(NotificationType notificationType) {
notificationsListeners.get(notificationType).clear();
}

Expand Down
Loading