Refact: Did nit fixes in entire project code and updated header also added missing headers #252

mnoman09 · 2019-01-02T18:57:23Z

Summary

Nit fixes in entire java sdk code and added missing headers

…added missing headers

coveralls · 2019-01-02T19:06:49Z

Pull Request Test Coverage Report for Build 822

350 of 376 (93.09%) changed or added relevant lines in 48 files are covered.
11 unchanged lines in 7 files lost coverage.
Overall coverage increased (+0.02%) to 89.788%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java	8	9	88.89%
core-api/src/main/java/com/optimizely/ab/bucketing/UserProfileUtils.java	0	1	0.0%
core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java	19	20	95.0%
core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java	11	12	91.67%
core-api/src/main/java/com/optimizely/ab/event/LogEvent.java	0	1	0.0%
core-api/src/main/java/com/optimizely/ab/event/internal/payload/Snapshot.java	3	4	75.0%
core-api/src/main/java/com/optimizely/ab/internal/EventTagUtils.java	2	3	66.67%
core-api/src/main/java/com/optimizely/ab/internal/ExperimentUtils.java	0	1	0.0%
core-api/src/main/java/com/optimizely/ab/OptimizelyRuntimeException.java	0	2	0.0%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	0	2	0.0%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/Optimizely.java	1	93.31%
core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java	1	93.13%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	1	84.21%
core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java	1	96.1%
core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java	2	97.66%
core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java	2	56.41%
core-api/src/main/java/com/optimizely/ab/internal/ConditionUtils.java	3	83.33%

Totals
Change from base Build 821:	0.02%
Covered Lines:	2708
Relevant Lines:	3016

💛 - Coveralls

mikeproeng37

lgtm, I just have one small comment

core-api/src/main/java/com/optimizely/ab/bucketing/UserProfileService.java

mikeproeng37 · 2019-01-02T19:33:32Z

...-api/src/main/java/com/optimizely/ab/notification/ActivateNotificationListenerInterface.java

+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */


Thanks for adding the missing header!

loganlinn

Thanks for the much needed clean-up. I left a few comments where I think we can do a little better while we are in here making changes.

Main thing we should clean-up is the placement of annotations on return types. I added a handful comments on specific lines, but not exhaustive. Can find more by searching the diff for "public @"

loganlinn · 2019-01-02T20:22:53Z

core-api/src/main/java/com/optimizely/ab/config/LiveVariable.java

@@ -117,7 +118,8 @@ public LiveVariable(@JsonProperty("id") String id,
        this.type = type;
    }

-    public @Nullable VariableStatus getStatus() {
+    public @Nullable
+    VariableStatus getStatus() {


this should be single line.

the cause is probably the annotation on the return type being unconventional. Snippet below is equivalent and probably wont confuse auto-formatter tool

@Nullable public VariableStatus getStatus() {

loganlinn · 2019-01-02T20:27:00Z

core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java

     * @param errorHandler the error handler to send exceptions to
     * @return the event type for the given event name
     */
-    public @CheckForNull EventType getEventTypeForName(String eventName, ErrorHandler errorHandler) {
+    public @CheckForNull
+    EventType getEventTypeForName(String eventName, ErrorHandler errorHandler) {


Please move the @CheckForNull annotation to before public like how its used on getExperimentForKey. They are equivalent and wont produce weird formatting where public is on different line. That's easy to misread as a package-local visibility.

loganlinn · 2019-01-02T20:27:16Z

core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java

-                getEventNameMapping()
-                        .get(eventName);
+            getEventNameMapping()
+                .get(eventName);


can be single line

loganlinn · 2019-01-02T20:28:16Z

core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java

@@ -299,7 +296,8 @@ public Experiment getExperimentForKey(@Nonnull String experimentKey,
    }


-    public @Nullable Experiment getExperimentForVariationId(String variationId) {
+    public @Nullable
+    Experiment getExperimentForVariationId(String variationId) {


Same as other comments about annotation placement for return types. This is formatting is worse.

loganlinn · 2019-01-02T20:28:36Z

core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java

-    public @Nullable Variation getForcedVariation(@Nonnull String experimentKey,
-                                                  @Nonnull String userId) {
+    public @Nullable
+    Variation getForcedVariation(@Nonnull String experimentKey,


Same as other comments about annotation placement for return types. This is formatting is worse.

loganlinn · 2019-01-02T20:30:10Z

core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java

@@ -67,7 +67,8 @@ public Object getValue() {
        return value;
    }

-    public @Nullable Boolean evaluate(ProjectConfig config, Map<String, ?> attributes) {
+    public @Nullable
+    Boolean evaluate(ProjectConfig config, Map<String, ?> attributes) {


Same as other comments about annotation placement for return types. This is formatting is worse.

loganlinn · 2019-01-02T20:30:14Z

core-api/src/main/java/com/optimizely/ab/config/audience/OrCondition.java

@@ -44,15 +44,15 @@ public OrCondition(@Nonnull List<Condition> conditions) {
    // false or null is null
    // false or false is false
    // null or null is null
-    public @Nullable Boolean evaluate(ProjectConfig config, Map<String, ?> attributes) {
+    public @Nullable
+    Boolean evaluate(ProjectConfig config, Map<String, ?> attributes) {


Same as other comments about annotation placement for return types. This is formatting is worse.

loganlinn · 2019-01-02T20:30:59Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/SubstringMatch.java

    protected SubstringMatch(String value) {
        this.value = value;
    }

    /**
     * This matches the same substring matching logic in the Web client.
+     *
     * @param attributeValue
     * @return true/false if the user attribute string value contains the condition string value
     */
    public @Nullable
    Boolean eval(Object attributeValue) {


Lets update this annotation placement for style consistency

loganlinn · 2019-01-02T20:31:17Z

core-httpclient-impl/src/main/java/com/optimizely/ab/event/AsyncEventHandler.java

+
+        @Override
+        public @CheckForNull
+        Void handleResponse(HttpResponse response) throws IOException {


Same as other comments about annotation placement for return types.

loganlinn · 2019-01-02T20:34:23Z

core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java

-    public @Nullable Variation bucket(@Nonnull Experiment experiment,
-                                      @Nonnull String bucketingId) {
+    public @Nullable
+    Variation bucket(@Nonnull Experiment experiment,


Same as other comments about annotation placement for return types. This is formatting is worse.

mnoman09 · 2019-01-02T21:12:48Z

@loganlinn I have updated the PR and did all the changes you have asked for.

Refact: Did nit fixes in entire project code and updated header also …

c9fd47d

…added missing headers

mikeproeng37 approved these changes Jan 2, 2019

View reviewed changes

Indentation in the code comment fix.

6b05688

loganlinn self-requested a review January 2, 2019 20:11

loganlinn suggested changes Jan 2, 2019

View reviewed changes

mnoman09 added 2 commits January 3, 2019 02:04

Nit fixes

be7600e

userprofile comment refact

fbe4a8f

loganlinn approved these changes Jan 2, 2019

View reviewed changes

Merge branch 'master' into mnoman/codeNitFixes

1578759

mikeproeng37 merged commit db15b83 into master Jan 7, 2019

mikeproeng37 deleted the mnoman/codeNitFixes branch January 15, 2019 22:29

Refact: Did nit fixes in entire project code and updated header also added missing headers #252

Refact: Did nit fixes in entire project code and updated header also added missing headers #252

Uh oh!

Conversation

mnoman09 commented Jan 2, 2019

Summary

Uh oh!

coveralls commented Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 822

💛 - Coveralls

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loganlinn 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!

mnoman09 commented Jan 2, 2019

Uh oh!

Uh oh!

coveralls commented Jan 2, 2019 •

edited

Loading