Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Settings: GetValueOrDefault<string>(key, null) returns empty string #8

Closed
jamesmontemagno opened this issue Jun 29, 2016 · 7 comments
Closed

Comments

@jamesmontemagno
Copy link
Owner

From @csoren on January 21, 2016 8:59

On iOS ISettings.GetValueOrDefault(key, null) returns null if the key does not exists. On Android, the same call returns the empty string, if the key does not exists.

I would expect the platforms to behave the same way and return the default value I specified (null).

Copied from original issue: jamesmontemagno/Xamarin.Plugins#195

@jamesmontemagno
Copy link
Owner Author

Agreed it should be consistent. Will require a bit of work because I need to check to see if the key exists and if not then return your default. This is what iOS does. I will say null is a bad idea and is actually an exception if you tried to pass it in to set it (well i convert it to string.empty). I highly recommend going with a different default.

@jamesmontemagno
Copy link
Owner Author

From @csoren on January 22, 2016 8:8

The reason I'm using null is so I can determine whether the key has been set. Passing a default value, such as the empty string, will not allow me to do that. There's no "KeyExists" function, if null is discouraged, perhaps the solution would include adding that to ISettings?

@jamesmontemagno
Copy link
Owner Author

From @dvshub on April 16, 2016 7:16

James, thanks for making this great effort to create a common Settings layer on all these platforms.

This issue (and the one I reported for AddOrUpdateValue) is actually pretty simple to fix. I debugged and found that the root cause is Convert.ToString(defaultValue) at Line# 96 and 219 on Android https://github.com/jamesmontemagno/Xamarin.Plugins/blob/1ed18269c3e123c67a1bd01685d0cdacd0ea4db3/Settings/Refractored.Xam.Settings.Android/Settings.cs

Since Convert.ToString() returns "The string representation of value, or String.Empty if value is an object whose value is null. If value is null, the method returns null."
as per https://msdn.microsoft.com/en-us/library/astxcyeh(v=vs.110).aspx

So the fix is:
defaultValue = (defaultValue == null)? null : Convert.ToString(defaultValue);

But this will result in removing the existing entry if the key was found since that is the default behavior of SharedPreferences on Android while adding an entry with null value.

Hence, the best solution is to say that null values are not supported by the Settings plugin and throw ArgumentException if the supplied value is null for case TypeCode.String. This will also make the Android behavior consistent with the behavior on iOS.

@jamesmontemagno
Copy link
Owner Author

I am officially marking it that we do not accept null as a value or nullables. In version 3.0 I will throw an exception when a null value is passed in.

@dvshub
Copy link

dvshub commented Oct 6, 2016

Awesome, thanks James.

On Wed, Oct 5, 2016 at 6:50 PM, James Montemagno notifications@github.com
wrote:

Closed #8 #8.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALnB3Hg03bS_kwRIGAokQnR3Ck8UtyQjks5qxFPwgaJpZM4JAxK_
.

@jamesmontemagno
Copy link
Owner Author

I wish I had a better solution, but nulls are just tricky and cause problems.

@dvshub
Copy link

dvshub commented Oct 6, 2016

Agreed. The solution looks good enough to me!

On Wed, Oct 5, 2016 at 8:33 PM, James Montemagno notifications@github.com
wrote:

I wish I had a better solution, but nulls are just tricky and cause
problems.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALnB3JooOGATimqsOKzJViki4Rytsn44ks5qxGv1gaJpZM4JAxK_
.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants