FabriBertani/Plugin.Maui.ScreenSecurity

Discussion of nullable refactoring class ScreenSecurity

Closed this issue · 1 comments

Change discussion

I wanted to make a suggestion for a change.

Is there a reason why the Property "Default" is nullable?
The Property "Default" can never return null in the current code.
And with the “SetDefault(IScreenSecurity? implementation)” method, it would also make no sense to pass null.
Since the method is internal, you should be able to change this without any problems. However, I have not found a reference to the method in the code. Perhaps it can also be removed.

What do you think about the proposed amendment?

Current code

public static class ScreenSecurity
{
    /// <summary>
    /// Provides the default implementation for static usage of this API.
    /// </summary>
    static IScreenSecurity? defaultImplementation;

    public static IScreenSecurity? Default => defaultImplementation ??= new ScreenSecurityImplementation();

    internal static void SetDefault(IScreenSecurity? implementation) => defaultImplementation = implementation;
}

Code suggestion

public static class ScreenSecurity
{
    /// <summary>
    /// Provides the default implementation for static usage of this API.
    /// </summary>
    static IScreenSecurity? defaultImplementation;

    public static IScreenSecurity Default => defaultImplementation ??= new ScreenSecurityImplementation();

    internal static void SetDefault(IScreenSecurity implementation) => defaultImplementation = implementation;
}

Hi @JarJarBinkz, I appreciate your suggestion but unfortunately, I'm going to reject it, let me explain why:

The Default property and the SetDefault method are currently designed to allow flexibility in initialization and reset behaviour, which explains why defaultImplementation is nullable.
Although the Default property itself never returns null due to the lazy initialization with ??= the nullability of defaultImplementation allows it to remain uninitialized until accessed or explicitly set. This pattern allows resetting the implementation using SetDefault(null). While the method is internal, keeping this flexibility ensures the class can adapt to future changes or use cases where resetting the implementation might be needed.
If the use case for passing null to SetDefault is deemed unnecessary, this change would also remove the ability to reset the implementation entirely, potentially limiting extensibility or debugging scenarios.

Thanks for raising this discussion and the improvement proposal.