Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TryGetValue() to IPreferences #26123

Open
maztan opened this issue Nov 26, 2024 · 13 comments
Open

Add TryGetValue() to IPreferences #26123

maztan opened this issue Nov 26, 2024 · 13 comments
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info good first issue Good for newcomers proposal/open t/enhancement ☀️ New feature or request
Milestone

Comments

@maztan
Copy link

maztan commented Nov 26, 2024

Description

Currently IPreferences allows checking if the property exists via ContainsKey(string key) method but it would be nice if it had a TryGetValue<T>(string propertyName, out T value)

Public API Changes

Public TryGetValue<T>(string propertyName, out T value) would be added to the IPreferences interface.

Intended Use-Case

More concise code.

@egvijayanand
Copy link
Contributor

Enables checking and retrieving the value in a single method call. Recommended practice for dictionaries.

@jfversluis
Copy link
Member

Get already has a default value for if its not found, so although the method name is different, that is basically implemented already? Or what am I missing?

@jfversluis jfversluis added s/needs-info Issue needs more info from the author area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info labels Nov 26, 2024
@maztan
Copy link
Author

maztan commented Nov 27, 2024

@jfversluis

I marked the line that would go away after the requested change was implemented. Also, current API forces providing default value each time the Get() is called, so this call should be preferably made only once within the range, where the preference's value is needed.

class DummyObservable
{
    IPreferences preferences = Preferences.Default;

    public string MyPref1
    {
        get => preferences.Get(nameof(MyPref1), "some default");
        set
        {
            if (preferences.ContainsKey(nameof(MyPref1)))
            {
                //THIS LINE WOULD GO AWAY AND PROVIDING DEFAULT VALUE SECOND TIME WOULDN'T BE NEEDED
                string currentValue = preferences.Get(nameof(MyPref1), "unneeded default");
                // if new value is null, check if currentValue is also null, in which case they are already equal (but can't call "Equals()" on null).
                if (!(value?.Equals(currentValue) ?? currentValue == null))
                {
                    preferences.Set(nameof(MyPref1), value);
                    OnPropertyChanged();
                }
            }
        }
    }

    public string MyPref2
    {
        get => preferences.Get(nameof(MyPref2), "David");
        set
        {
            if (preferences.TryGetValue(nameof(MyPref2), out string currentValue))
            {
                if (!(value?.Equals(currentValue) ?? currentValue == null))
                {
                    preferences.Set(nameof(MyPref2), value);
                    OnPropertyChanged();
                }
            }
        }
    }

    public event PropertyChangedEventHandler? PropertyChanged;

    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = "")
    {
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
    }
}

MyPref2 uses the method TryGetValue that does not currently exist and is the one requested for implementation.

@dotnet-policy-service dotnet-policy-service bot added s/needs-attention Issue has more information and needs another look and removed s/needs-info Issue needs more info from the author labels Nov 27, 2024
@jfversluis
Copy link
Member

But you don't need if (preferences.ContainsKey(nameof(MyPref1))) because that is already implicit with the default value. If the key doesn't exist, it will return the default value. So in that case the value will never be null and you don't need those checks.

I mean, I see some value, but its mostly writing slightly different code to achieve the same thing you can already do today.

@jfversluis jfversluis added t/enhancement ☀️ New feature or request good first issue Good for newcomers and removed s/needs-attention Issue has more information and needs another look labels Nov 27, 2024
@jfversluis jfversluis added this to the Backlog milestone Nov 27, 2024
@maztan
Copy link
Author

maztan commented Nov 27, 2024

But you don't need if (preferences.ContainsKey(nameof(MyPref1))) because that is already implicit with the default value. If the key doesn't exist, it will return the default value. So in that case the value will never be null and you don't need those checks.

I mean, I see some value, but its mostly writing slightly different code to achieve the same thing you can already do today.

If this was true, the ContainsKey method could be removed from the API altogether. If the default value is null, the call to ContainsKey is still required. Also, this is just one example of potential usage.

@maztan
Copy link
Author

maztan commented Nov 27, 2024

For now, I've added this extension method to my codebase. It does not improve performance what a "real" TryGetValue implementation might be able to do, but it matches the functionality.

public static bool TryGetValue<T>(this IPreferences _this, string key, out T? value)
{
    if (_this.ContainsKey(key))
    {
        value = _this.Get<T?>(key, default);
        return true;
    }

    value = default;
    return false;
}

@egvijayanand
Copy link
Contributor

The TryGetValue() method has a subtle advantage because it always returns a Boolean value, making it easy to apply in various scenarios.

@jeremi-przeorek
Copy link

I would like to pick this up if that's alright ✋

@jeremi-przeorek
Copy link

@egvijayanand how do I navigate to pick this up?

@egvijayanand
Copy link
Contributor

@egvijayanand how do I navigate to pick this up?

You need to get approval for this work item from any one of the maintainers of this repo.

@maztan
Copy link
Author

maztan commented Feb 2, 2025

@jeremi-przeorek you need to check out the repository an create a pull request when you are done with the coding

@pictos
Copy link
Contributor

pictos commented Feb 2, 2025

IMHO, It's really easy to add this as an extension method, so devs can implement it by their own if needed and there's no need to add a new API for that on maui

@maztan
Copy link
Author

maztan commented Feb 2, 2025

@pictos The existing API is awkward. I would create a PR and see what happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info good first issue Good for newcomers proposal/open t/enhancement ☀️ New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants