search

pwittchen / prefser

Wrapper for Android SharedPreferences with object serialization and RxJava Observables
Apache License 2.0
229 stars 26 forks source link

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

Open LeonardooBorges opened 7 years ago

LeonardooBorges commented 7 years ago

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.

codecov-io commented 7 years ago

Codecov Report

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

Impacted file tree graph

@@              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%> (ø) :arrow_up:
.../com/github/pwittchen/prefser/library/Prefser.java 98.83% <100%> (-0.04%) :arrow_down:

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 commented 7 years ago

Hi,

Thanks for your contribution. I'll review that.

07.07.2017 8:11 PM "LeonardooBorges" notifications@github.com napisał(a):

50 https://github.com/pwittchen/prefser/pull/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 https://github.com/LeonardooBorges/prefser/commit/2bb0108dfaf06f92e2285496f622a2c74e21a065 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:

https://github.com/pwittchen/prefser/pull/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

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pwittchen/prefser/pull/114, or mute the thread https://github.com/notifications/unsubscribe-auth/AAqcF38d15AHwy7AbEbh1ZteGGv_TLcJks5sLnSCgaJpZM4ORQ2f .

pwittchen commented 7 years ago

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 commented 7 years ago

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.

  1. 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.

  2. 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 commented 7 years ago

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 commented 7 years ago

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