Refact: Added builder pattern in decision listener #275

mnoman09 · 2019-03-22T16:30:11Z

Summary

Refactored FeatureVariable Decision listener PR and applied builder pattern
Updated notification listener to directly send notification from it and added its method

Once these changes will get approved than we can move forward and we can apply these changes in all decision listener PRs

…de constants insead of enum

…tification from notification center

msohailhussain · 2019-03-22T17:50:58Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

     * @return true if removed otherwise false (if the notification is already registered, it returns false).
     */
    public boolean removeNotificationListener(int notificationID) {
+        for (NotificationHolder holder : decisionListenerHolder) {


@mikecdavis What do you suggest here? There is separate logic to remove for DecisionListener object.

/** * 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 removeNotificationListener(int notificationID) { for (ArrayList<NotificationHolder> notificationHolders: notificationsListeners.values()) { if (removeNotificationListener(notificationHolders)) { return true; } } return removeNotificationListener(decisionListenerHolder); } /** * Helper method to iterate find NotificationHolder in an List identified by by the notificationId */ private static final public boolean removeNotificationListener(int notificationID, List<NotificationHolder>) { for (NotificationHolder holder : decisionListenerHolder) { if (holder.notificationId != notificationID) { continue; } notificationsListeners.remove(holder); logger.info("Notification listener removed {}", notificationID); return true; } }

mikecdavis

Much better thank you! There are a few more tweaks I'd like to see, and then one open question regarding a new normal for notification going forward.

mikecdavis · 2019-03-22T18:57:26Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

-                public void onDecision(@Nonnull String type, @Nonnull String userId, @Nonnull Map<String, ?> attributes, @Nonnull Map<String, ?> decisionInfo) {
-                    decisionNotificationListenerInterface.onDecision(type, userId, attributes, decisionInfo);
+    public int addDecisionNotificationListener(DecisionNotificationListener decisionNotificationListener) {
+        notificationListenerID++;


We should replace int notificationListenerID with an AtomicInteger and use the incrementAndGet() method.
https://stackoverflow.com/questions/25168062/why-is-i-not-atomic

mikecdavis · 2019-03-22T19:12:30Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

     * @return true if removed otherwise false (if the notification is already registered, it returns false).
     */
    public boolean removeNotificationListener(int notificationID) {
+        for (NotificationHolder holder : decisionListenerHolder) {


/** * 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 removeNotificationListener(int notificationID) { for (ArrayList<NotificationHolder> notificationHolders: notificationsListeners.values()) { if (removeNotificationListener(notificationHolders)) { return true; } } return removeNotificationListener(decisionListenerHolder); } /** * Helper method to iterate find NotificationHolder in an List identified by by the notificationId */ private static final public boolean removeNotificationListener(int notificationID, List<NotificationHolder>) { for (NotificationHolder holder : decisionListenerHolder) { if (holder.notificationId != notificationID) { continue; } notificationsListeners.remove(holder); logger.info("Notification listener removed {}", notificationID); return true; } }

mikecdavis · 2019-03-22T19:13:52Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

+    public void sendNotifications(DecisionNotification decision) {
+        for (NotificationHolder holder : decisionListenerHolder) {
+            try {
+                (holder.decisionNotificationListener).onDecision(decision);


Remove the parenthesis holder.decisionNotificationListener.onDecision(decision);

mikecdavis · 2019-03-22T19:18:55Z

...i/src/main/java/com/optimizely/ab/notification/decisionInfo/FeatureVariableNotification.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class FeatureVariableNotification extends DecisionNotification {


This is a pure builder. This does not need to extend DecisionNotification nor do we need a Builder subclass. Refactor as FeatureVariableDecisionNotificationBuilder a bit wordy, I know, but accurate.

Actually let's subclass under DecisionNotification so the interface will be:

DecisionNotification decisionNotification = DecisionNotification.newFeatureVariableBuilder() ... .build();

Other "types" can be additional sub-classes.

mikecdavis · 2019-03-22T19:22:06Z

core-api/src/main/java/com/optimizely/ab/notification/decisionInfo/DecisionNotification.java

 ***************************************************************************/

-package com.optimizely.ab.notification;
+package com.optimizely.ab.notification.decisionInfo;


Relocate this back under com.optimizely.ab.notification nesting packages is often less ideal.

mikecdavis · 2019-03-22T23:12:17Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotificationListener.java

-                                    @Nonnull String userId,
-                                    @Nonnull Map<String, ?> attributes,
-                                    @Nonnull Map<String, ?> decisionInfo);
+    void onDecision(@Nonnull DecisionNotification decisionNotification);


I've been trying to see if we can use generics to maintain some higher order generality. Ideally NotificationLister originally would have been defined like:

public interface NotificationListener<T> { void notify(T notification); }

but since it wasn't trying to think the best way forward.

Yes, you are right we can use generics but I think than we have to refactor Track and Activate listeners as well. If I am understanding this correctly?

loganlinn · 2019-03-26T18:12:16Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java

+            decisionInfo.put(VARIABLE_KEY, variableKey);
+            decisionInfo.put(VARIABLE_TYPE, variableType);
+            decisionInfo.put(VARIABLE_VALUE, variableValue);
+            if (featureDecision.decisionSource != null && featureDecision.decisionSource.equals(FeatureDecision.DecisionSource.EXPERIMENT)) {


Suggested change

if (featureDecision.decisionSource != null && featureDecision.decisionSource.equals(FeatureDecision.DecisionSource.EXPERIMENT)) {

if (featureDecision != null && FeatureDecision.DecisionSource.EXPERIMENT.equals(featureDecision.decisionSource)) {

loganlinn · 2019-03-26T18:13:54Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java

+            return this;
+        }
+
+        public FeatureVariableDecisionNotificationBuilder withFeatureEnabled(Boolean featureEnabled) {


Suggested change

public FeatureVariableDecisionNotificationBuilder withFeatureEnabled(Boolean featureEnabled) {

public FeatureVariableDecisionNotificationBuilder withFeatureEnabled(boolean featureEnabled) {

loganlinn · 2019-03-26T18:16:51Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java

+                decisionInfo.put(SOURCE, FeatureDecision.DecisionSource.ROLLOUT);
+            }
+
+            this.type = NotificationCenter.DecisionNotificationType.FEATURE_VARIABLE.toString();


If type is not something that's settable, it's more clear if it's set in the constructor. Maybe it should even be parameter of BaseDecisionNotificationBuilder constructor.

loganlinn · 2019-03-26T18:25:20Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java

+
+            this.type = NotificationCenter.DecisionNotificationType.FEATURE_VARIABLE.toString();
+
+            return super.build();


It doesn't seem like the builder class is buying us much and it makes this code harder to follow (stateful logic & having to jump between two classes). Would be more straightforward to just instantiate here

return new DecisionNotification( type, userId, attributes, decisionInfo);

Regarding the defaulting of attributes = new HashMap<>(), that can be done at field declaration on this class or in the DecisionNotification constructor.

loganlinn · 2019-03-26T18:27:25Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java

+        this.type = type;
+        this.userId = userId;
+        this.attributes = attributes;
+        this.decisionInfo = decisionInfo;


Are all of these fields allowed to be null? I see in BaseDecisionNotificationBuilder, that we default attributes to empty map -- that should be moved here (otherwise assert non-null in constructor) to make it consistent, otherwise nulls can slip in.

loganlinn · 2019-03-26T18:35:41Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

-                public void onDecision(@Nonnull String type, @Nonnull String userId, @Nonnull Map<String, ?> attributes, @Nonnull Map<String, ?> decisionInfo) {
-                    decisionNotificationListenerInterface.onDecision(type, userId, attributes, decisionInfo);
+    public int addDecisionNotificationListener(DecisionNotificationListener decisionNotificationListener) {
+        int id = this.notificationListenerID.incrementAndGet();


Consider moving this call to where id is actually needed. Since there are early-returns in this method, we could bump the id sequence unnecessarily.

loganlinn · 2019-03-26T18:42:45Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

-            if (holder.notificationListener == notificationListener ) {
+            if (holder.notificationListener == notificationListener) {
                logger.warn("Notification listener was already added");
                return -1;


I see that returning -1 is existing behavior. It's misleading and worth changing IMO. Maybe a later PR, but maybe add a TODO

loganlinn · 2019-03-26T18:43:04Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

-                }
+
+        for (ArrayList<NotificationHolder> notificationHolders : notificationsListeners.values()) {
+            if(removeNotificationListener(notificationID, notificationHolders)) {


Suggested change

if(removeNotificationListener(notificationID, notificationHolders)) {

if (removeNotificationListener(notificationID, notificationHolders)) {

loganlinn · 2019-03-26T18:43:17Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

-                    return true;
-                }
+
+        for (ArrayList<NotificationHolder> notificationHolders : notificationsListeners.values()) {


Suggested change

for (ArrayList<NotificationHolder> notificationHolders : notificationsListeners.values()) {

for (List<NotificationHolder> notificationHolders : notificationsListeners.values()) {

loganlinn · 2019-03-26T18:44:26Z

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

+     * @param notificationHolderList list from which to remove notification listener.
+     * @return true if removed otherwise false
+     */
+    private boolean removeNotificationListener(int notificationID, ArrayList<NotificationHolder> notificationHolderList) {


Suggested change

private boolean removeNotificationListener(int notificationID, ArrayList<NotificationHolder> notificationHolderList) {

private boolean removeNotificationListener(int notificationID, List<NotificationHolder> notificationHolderList) {

side-note: a Map, like LinkedHashMap, keyed by id would make things easier but also retain ordering...

loganlinn · 2019-03-28T01:36:24Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java

            decisionInfo.put(VARIABLE_TYPE, variableType);
            decisionInfo.put(VARIABLE_VALUE, variableValue);
-            if (featureDecision.decisionSource != null && featureDecision.decisionSource.equals(FeatureDecision.DecisionSource.EXPERIMENT)) {
+            if (featureDecision.decisionSource != null && FeatureDecision.DecisionSource.EXPERIMENT.equals(featureDecision.decisionSource)) {


Potential NullPointerException if featureDecision is null

mikecdavis

Minor remaining comment, and I think you should consider @loganlinn comments as well.

mikecdavis · 2019-03-28T22:49:23Z

core-api/src/main/java/com/optimizely/ab/notification/DecisionNotification.java

+        return new FeatureVariableDecisionNotificationBuilder();
+    }
+
+    private static class BaseDecisionNotificationBuilder {


This base class is not needed.

aliabbasrizvi · 2019-03-29T17:57:17Z

@loganlinn can you take another look?

aliabbasrizvi

LGTM

mnoman09 added 2 commits March 21, 2019 21:57

Applied BuilderPattern On getFeatureVariable decision listener and ma…

0b51b15

…de constants insead of enum

Removed notification listener interface class and directly sending no…

a28e17c

…tification from notification center

mnoman09 requested review from aliabbasrizvi and mikecdavis March 22, 2019 16:30

msohailhussain reviewed Mar 22, 2019

View reviewed changes

msohailhussain requested a review from a team March 22, 2019 18:11

mikecdavis suggested changes Mar 22, 2019

View reviewed changes

Refact

03b7589

mnoman09 requested review from mikecdavis and msohailhussain and removed request for msohailhussain March 25, 2019 14:37

reverting : returning default variable if featureEnabled is false

0972ac3

aliabbasrizvi requested a review from msohailhussain March 26, 2019 17:55

loganlinn reviewed Mar 26, 2019

View reviewed changes

Nit fixes

ef3bdbe

mnoman09 requested a review from loganlinn March 27, 2019 12:41

loganlinn reviewed Mar 28, 2019

View reviewed changes

Added check of featureDecision is not null

f0016a9

mnoman09 requested a review from loganlinn March 28, 2019 13:53

mikecdavis approved these changes Mar 28, 2019

View reviewed changes

Removed base class of decision notification

db4cb66

aliabbasrizvi approved these changes Mar 29, 2019

View reviewed changes

aliabbasrizvi merged commit 158bee4 into mnoman/getFeatVarListener Apr 1, 2019

aliabbasrizvi deleted the mnoman/BuildPatrnOnDecision branch April 1, 2019 18:35

	if (featureDecision.decisionSource != null && featureDecision.decisionSource.equals(FeatureDecision.DecisionSource.EXPERIMENT)) {
	if (featureDecision != null && FeatureDecision.DecisionSource.EXPERIMENT.equals(featureDecision.decisionSource)) {

	public FeatureVariableDecisionNotificationBuilder withFeatureEnabled(Boolean featureEnabled) {
	public FeatureVariableDecisionNotificationBuilder withFeatureEnabled(boolean featureEnabled) {


		this.type = NotificationCenter.DecisionNotificationType.FEATURE_VARIABLE.toString();

		return super.build();

	if(removeNotificationListener(notificationID, notificationHolders)) {
	if (removeNotificationListener(notificationID, notificationHolders)) {

	for (ArrayList<NotificationHolder> notificationHolders : notificationsListeners.values()) {
	for (List<NotificationHolder> notificationHolders : notificationsListeners.values()) {

	private boolean removeNotificationListener(int notificationID, ArrayList<NotificationHolder> notificationHolderList) {
	private boolean removeNotificationListener(int notificationID, List<NotificationHolder> notificationHolderList) {

Refact: Added builder pattern in decision listener #275

Refact: Added builder pattern in decision listener #275

Uh oh!

Conversation

mnoman09 commented Mar 22, 2019 • edited by aliabbasrizvi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikecdavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aliabbasrizvi commented Mar 29, 2019

Uh oh!

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mnoman09 commented Mar 22, 2019 •

edited by aliabbasrizvi

Loading