Feature/support null as default value (primitive types) #114

LeonardooBorges · 2017-07-07T18:10:10Z

Passing null as default value in get gives a NullPointerException when the key is already signed for primitive types. From my study of the code I see that it was passing the defValue (object of primitive) to SharedPreferences but it only accepts primitive types. This way sending a null gives the exception. I saw your comment in the #50 issue, but I think that sending null as default value make sense sometimes, even if using the primitive objects.
I also added some tests, you can run them in your version to see the problem by yourself.
You can see just the first commit also, it already solves the issue, but the second one improves the readability and comprehension of the changes.

…hen getting a primitive type.

… the readability and comprehension of the change of first commit.

codecov-io · 2017-07-07T18:19:27Z

Codecov Report

Merging #114 into RxJava1.x will decrease coverage by 0.03%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##           RxJava1.x     #114      +/-   ##
=============================================
- Coverage      98.11%   98.07%   -0.04%     
=============================================
  Files              5        5              
  Lines            159      156       -3     
  Branches          11       10       -1     
=============================================
- Hits             156      153       -3     
  Misses             2        2              
  Partials           1        1

Impacted Files	Coverage Δ
.../prefser/library/PreferencesAccessorsProvider.java	`100% <100%> (ø)`	⬆️
.../com/github/pwittchen/prefser/library/Prefser.java	`98.83% <100%> (-0.04%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63cff55...c10455f. Read the comment docs.

pwittchen · 2017-07-07T20:00:15Z

Hi, Thanks for your contribution. I'll review that. 07.07.2017 8:11 PM "LeonardooBorges" <notifications@github.com> napisał(a):

…

#50 <#50> Passing null as default value in get gives a NullPointerException when the key is already signed for primitive types. From my study of the code I see that it was passing the defValue (object of primitive) to SharedPreferences but it only accepts primitive types. This way sending a null gives the exception. I saw your comment in the 50 issue, but I think that sending null as default value make sense sometimes, even if using the primitive objects. I also added some tests, you can run them in your version to see the problem by yourself. You can see just the first commit <LeonardooBorges@2bb0108> also, it already solves the issue, but the second one improves the readability and comprehension of the changes. ------------------------------ You can view, comment on, or merge this pull request online at: #114 Commit Summary - Add some tests and changes to support sending null as default value when getting a primitive type. - Changes to verify the existence of the key before accessor, improving the readability and comprehension of the change of first commit. File Changes - *M* build.gradle <https://github.com/pwittchen/prefser/pull/114/files#diff-0> (2) - *M* library/src/main/java/com/github/pwittchen/prefser/ library/Accessor.java <https://github.com/pwittchen/prefser/pull/114/files#diff-1> (2) - *M* library/src/main/java/com/github/pwittchen/prefser/library/ PreferencesAccessorsProvider.java <https://github.com/pwittchen/prefser/pull/114/files#diff-2> (24) - *M* library/src/main/java/com/github/pwittchen/prefser/ library/Prefser.java <https://github.com/pwittchen/prefser/pull/114/files#diff-3> (18) - *M* library/src/test/java/com/github/pwittchen/prefser/ library/PrefserTest.java <https://github.com/pwittchen/prefser/pull/114/files#diff-4> (175) Patch Links: - https://github.com/pwittchen/prefser/pull/114.patch - https://github.com/pwittchen/prefser/pull/114.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#114>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAqcF38d15AHwy7AbEbh1ZteGGv_TLcJks5sLnSCgaJpZM4ORQ2f> .

LeonardooBorges · 2017-07-07T20:12:14Z

library/src/test/java/com/github/pwittchen/prefser/library/PrefserTest.java

+    assertThat(givenValue).isEqualTo(readValue);
+  }
+
+  // alteracoes comeca aqui


Sorry for that comment, after your review I can delete it.

This comment should be deleted.

LeonardooBorges · 2017-07-07T20:12:46Z

library/src/test/java/com/github/pwittchen/prefser/library/PrefserTest.java

+    // then
+    assertThat(value).isNull();
+//    CustomClass givenObject = new CustomClass(23, "someText");
+//    CustomClass defaultObject = new CustomClass(67, "defaultText");


Commented code should be removed

pwittchen · 2017-07-08T20:06:34Z

library/src/test/java/com/github/pwittchen/prefser/library/PrefserTest.java

+    // then
+    assertThat(value).isNull();
+//    CustomClass givenObject = new CustomClass(23, "someText");
+//    CustomClass defaultObject = new CustomClass(67, "defaultText");


Commented code should be removed

pwittchen · 2017-07-08T20:06:54Z

library/src/test/java/com/github/pwittchen/prefser/library/PrefserTest.java

+    assertThat(givenValue).isEqualTo(readValue);
+  }
+
+  // alteracoes comeca aqui


This comment should be deleted.

pwittchen · 2017-07-08T20:08:47Z

library/src/main/java/com/github/pwittchen/prefser/library/PreferencesAccessorsProvider.java

@@ -48,7 +48,7 @@ private void createAccessors() {
  private void createBooleanAccessor() {
    accessors.put(Boolean.class, new Accessor<Boolean>() {
      @Override public Boolean get(String key, Boolean defaultValue) {
-        return preferences.getBoolean(key, defaultValue);
+        return preferences.getBoolean(key, (defaultValue == null) ? false : defaultValue);


In this use case, we are defining default behavior for the user in the case of passing null. We don't know if a user wants true or false, but we're forcing using one of these. The same rule applies to all of the changes below.

No, we are not defining default behavior for the user, because if the user sends null in defValue, it would already return before in Prefser class (and it would return null).

Why would user pass the null? Why should the user expect false in the case of passing null, but not true? How could he or she guess such behavior just by looking at the library API without browsing source code or debugging?

The user will not get true or false if he/she passing null, the user will get null. Look the other comment I mention you, we are returning defValue before that check.

if (!contains(key)) { return defaultValue; }

Line 250 of Prefser.

pwittchen · 2017-07-08T20:08:59Z

library/src/main/java/com/github/pwittchen/prefser/library/PreferencesAccessorsProvider.java

@@ -60,7 +60,7 @@ private void createBooleanAccessor() {
  private void createFloatAccessor() {
    accessors.put(Float.class, new Accessor<Float>() {
      @Override public Float get(String key, Float defaultValue) {
-        return preferences.getFloat(key, defaultValue);
+        return preferences.getFloat(key, (defaultValue == null) ? 0f : defaultValue);


Same issue as above.

pwittchen · 2017-07-08T20:09:03Z

library/src/main/java/com/github/pwittchen/prefser/library/PreferencesAccessorsProvider.java

@@ -72,7 +72,7 @@ private void createFloatAccessor() {
  private void createIntegerAccessor() {
    accessors.put(Integer.class, new Accessor<Integer>() {
      @Override public Integer get(String key, Integer defaultValue) {
-        return preferences.getInt(key, defaultValue);
+        return preferences.getInt(key, (defaultValue == null) ? 0 : defaultValue);


Same issue as above.

pwittchen · 2017-07-08T20:09:07Z

library/src/main/java/com/github/pwittchen/prefser/library/PreferencesAccessorsProvider.java

@@ -84,7 +84,7 @@ private void createIntegerAccessor() {
  private void createLongAccessor() {
    accessors.put(Long.class, new Accessor<Long>() {
      @Override public Long get(String key, Long defaultValue) {
-        return preferences.getLong(key, defaultValue);
+        return preferences.getLong(key,  (defaultValue == null) ? 0L : defaultValue);


Same issue as above.

pwittchen · 2017-07-08T20:09:57Z

library/src/main/java/com/github/pwittchen/prefser/library/Accessor.java

@@ -16,7 +16,7 @@
 package com.github.pwittchen.prefser.library;

 interface Accessor<T> {
-  T get(String key, T defaultValue);
+  T get(String key);


I'd leave default value as a parameter inside the Accessor interface

pwittchen · 2017-07-08T20:20:42Z

Hello @LeonardooBorges. I really appreciate your contribution, but I'd generally avoid forcing default behavior for accessing data. I think, in the case of this library, the user should explicitly define such behavior. Default Android SharedPreferences mechanism has almost the same API and behavior. Moreover, you wrote:

that sending null as default value make sense sometimes, even if using the primitive objects.

Can you provide any reasonable examples for that? I think passing null as a parameter or returning is generally a bad practice mentioned by the Uncle Bob in "Clean Code" book. If we can avoid it, we should do it. If the user provides null as a default value, he or she should expect NPE in some places if there will be no null-checks. We can add @NonNull annotation for default values or additional validation, but on the certain level, after passing null, the Exception will be thrown anyway. In addition, in the current design of the library, this behavior is correct.

PS. It's easier to review code when it's squashed into the single commit within the single PR. You can take a look at this tool: http://rebaseandsqua.sh/.

LeonardooBorges · 2017-07-10T15:37:20Z

library/src/main/java/com/github/pwittchen/prefser/library/Prefser.java

@@ -253,20 +247,20 @@ public boolean contains(String key) {
    Preconditions.checkNotNull(key, KEY_IS_NULL);
    Preconditions.checkNotNull(typeTokenOfT, TYPE_TOKEN_OF_T_IS_NULL);

+    if (!contains(key)) {
+      return defaultValue;


@pwittchen here we are returning the defValue, so if the execution reaches there (return preferences.getBoolean(key, (defaultValue == null) ? false : defaultValue);) SharedPreferences will contain the key and defValue will not be used.

LeonardooBorges · 2017-07-10T18:10:38Z

Hello @pwittchen it is me that should be thanking you for what you did and share, many projects in the company I work uses your lib and we are thankful. I already fork and I'm using the changes I have done and as it works fine I decide to open this PR to contribute. I have some points to Prefser accept the null as defValue.

Prefser can receive and return any object (because it is generics) and null is a value that any object can accept, so the lib could do this also.
I wrote some generics code and the default null value was a good option to check if the app had already set some key, but in the way it is I can't, because an exception is thrown in SharedPreferences. I rather receive null (and it actually is the default value in the case) instead of get an exception in return.

The changes I did increases the Prefser scope to accept more values, so I cannot see a reason to not accept that. Please lets continue the discussion if you not agree.

pwittchen · 2017-07-10T19:59:23Z

Ok, I see your point :). In the case of Java generics, we're not able to return null and NPE will be thrown. Nevertheless, I would prefer more elegant solution than simply returning some predefined values. I'd prefer to have some kind of Optional or Null Object Pattern implementation to make returned values more straightforward. I just don't want to hide "magic logic" from the users. I also don't want to add Guava dependency to this library just to add one Optional. I have to think about a possible solution that would be fine & consistent with my idea of the library code. Maybe I'll create some PoC and share it here later.

LeonardooBorges · 2017-07-10T20:20:17Z

Thanks for your time, if you could share your solution later I will appreciate to see. 😄

LeonardooBorges added 2 commits July 7, 2017 14:10

Add some tests and changes to support sending null as default value w…

2bb0108

…hen getting a primitive type.

Changes to verify the existence of the key before accessor, improving…

c10455f

… the readability and comprehension of the change of first commit.

LeonardooBorges changed the title ~~Feature/support default value as null~~ Feature/support null as default value (primitive types) Jul 7, 2017

LeonardooBorges commented Jul 7, 2017

View reviewed changes

pwittchen reviewed Jul 8, 2017

View reviewed changes

LeonardooBorges commented Jul 10, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/support null as default value (primitive types) #114

Feature/support null as default value (primitive types) #114

LeonardooBorges commented Jul 7, 2017 •

edited by pwittchen

Loading

codecov-io commented Jul 7, 2017

pwittchen commented Jul 7, 2017 via email

LeonardooBorges Jul 7, 2017

pwittchen Jul 8, 2017

LeonardooBorges Jul 7, 2017

pwittchen Jul 8, 2017

pwittchen Jul 8, 2017

pwittchen Jul 8, 2017

pwittchen Jul 8, 2017

LeonardooBorges Jul 10, 2017

pwittchen Jul 10, 2017 •

edited

Loading

LeonardooBorges Jul 10, 2017

pwittchen Jul 8, 2017

pwittchen Jul 8, 2017

pwittchen Jul 8, 2017

pwittchen Jul 8, 2017

pwittchen commented Jul 8, 2017 •

edited

Loading

LeonardooBorges Jul 10, 2017

LeonardooBorges commented Jul 10, 2017

pwittchen commented Jul 10, 2017 •

edited

Loading

LeonardooBorges commented Jul 10, 2017

Feature/support null as default value (primitive types) #114

Are you sure you want to change the base?

Feature/support null as default value (primitive types) #114

Conversation

LeonardooBorges commented Jul 7, 2017 • edited by pwittchen Loading

codecov-io commented Jul 7, 2017

Codecov Report

pwittchen commented Jul 7, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittchen Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittchen commented Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

LeonardooBorges commented Jul 10, 2017

pwittchen commented Jul 10, 2017 • edited Loading

LeonardooBorges commented Jul 10, 2017

LeonardooBorges commented Jul 7, 2017 •

edited by pwittchen

Loading

pwittchen Jul 10, 2017 •

edited

Loading

pwittchen commented Jul 8, 2017 •

edited

Loading

pwittchen commented Jul 10, 2017 •

edited

Loading