jamesmontemagno / Xamarin.Plugins

Cross-platform Native API Access from Shared Code!
MIT License
1.3k stars 380 forks source link

Settings: GetValueOrDefault("key", new MyClass()) throws NullReferenceException instead of ArgumentException. #287

Closed dvshub closed 8 years ago

dvshub commented 8 years ago

Please take a moment to fill out the following (change to preview to check or place x in []):

This is a

Which plugin does this impact:

Version Number of Plugin: 2.1.0 Device Tested On: Nexus 6P, Android version 6.0.1 Simulator Tested On: iPhone 6s, iOS version 9.2

Expected Behavior

Invoking Settings.GetValueOrDefault(key, T defaultValue) for a custom reference type or a custom value type should throw ArgumentException with the message "Value of type MyClass is not supported."

Actual Behavior

Settings.GetValueOrDefault("key", new MyClass()) throws NullReferenceException on both Android and iOS.

Steps to reproduce the Behavior

// Dummy class for testing unsupported reference types. private class MyClass {} // Dummy struct for testing unsupported value types. private struct MyStruct {}

Settings.GetValueOrDefault("key", new MyClass()); // throws NullReferenceException Settings.GetValueOrDefault("key", new MyStruct()); // throws NullReferenceException

This is because: On Android https://github.com/jamesmontemagno/Xamarin.Plugins/blob/1ed18269c3e123c67a1bd01685d0cdacd0ea4db3/Settings/Refractored.Xam.Settings.Android/Settings.cs At line# 171: value is null, hence value.GetType().Name throws NullReferenceException

On iOS https://github.com/jamesmontemagno/Xamarin.Plugins/blob/1ed18269c3e123c67a1bd01685d0cdacd0ea4db3/Settings/Refractored.Xam.Settings.iOS/Settings.cs At line# 116: value is null, hence value.GetType().Name throws NullReferenceException

Can be fixed on both platforms by using defaultValue.GetType().Name instead of value.GetType().Name

jamesmontemagno commented 8 years ago

Settings supports: Data Types Supported

Boolean Int32 Int64 String Single(float) Guid Double Decimal DateTime

You can not save complex classes.

dvshub commented 8 years ago

Yes, but shouldn't you be throwing ArgumentException instead of NullReferenceException per your source code which tries to throw ArgumentException, but it actually crashes because value is null. I think that should be fixed.