Abstracted Interface for Settings

I just learned about the beauties and wonders of interfaces and I realized how I could fix some code that has been bothering me because of its duplication.

This is my ISettingsProvider, SettingChangedEventArgs class, and ApplicationSettingProvider class, which are in the same file:

public class SettingChangedEventArgs : EventArgs
{
    public readonly int NewSetting;

    public SettingChangedEventArgs(int newSetting)
    {
        NewSetting = newSetting;
    }
}

public interface ISettingsProvider
{
    int GetCurrentSetting();
    void SetCurrentSetting(int theme);
    event EventHandler SettingChanged;
}

public abstract class ApplicationSettingProvider : ISettingsProvider
{
    public abstract int GetCurrentSetting();
    public abstract void SetCurrentSetting(int setting);

    public event EventHandler SettingChanged;
    public void OnSettingChanged(int newSetting)
    {
        var handler = SettingChanged;
        if (handler != null)
        {
            handler(this, new SettingChangedEventArgs(newSetting));
        }
    }
}

ApplicationThemeProvider:

```
public class ApplicationThemeProvider : ApplicationSettingProvider
{
public ApplicationThemeProvider()
{
ApplicationData.Current.DataChanged += (a, o) =>
{
CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
{
OnSettingChanged((int)ApplicationData.Current.RoamingSettings.Values["Theme"]);
});
};
}

public override int GetCurrentSetting()
{
if (ApplicationData.Current.RoamingSettings.Values.ContainsKey("Theme"))
{
int value = (int)ApplicationData.Current.RoamingSettings.Values["Theme"];
return (value >= 0 && value <= 2) ? value : 0;
}
return 0;
}

public override void SetCurrentSetting(int theme)
{
ApplicationData.Current.RoamingSettings.Values["Theme"] = theme;
Ap

public readonly int NewSetting;

public SettingChangedEventArgs(int newSetting)
{
    NewSetting = newSetting;
}

Notice how the code formatting is getting confused with NewSetting? It might be readonly, but it's still a public field you have here. An EventArgs class is a class like any other - there's no reason to not properly encapsulate its data. I like my immutable stuff backed with an explicit readonly field, so I'd write it like this:

private readonly int _newSetting;
public int NewSetting { get { return _newSetting; } }

public SettingChangedEventArgs(int newSetting)
{
    _newSetting = newSetting;
}

The alternative is to use an auto-property:

NewSetting { get; private set; }

...but that doesn't quite convey immutability as well as a readonly backing field, in my opinion.

The idea behind writing an interface, is generally to code against an abstraction - so you can code against an interface, but then an abstract class is also an abstraction: making an abstract class implement an interface is always something I get second thoughts about - do I really need that abstract class? If so, do I really need that interface?

public interface ISettingsProvider
{
    int GetCurrentSetting();
    void SetCurrentSetting(int theme);
    event EventHandler SettingChanged;
}

So we have an abstraction representing a settings provider. Let's see what the abstract class is buying us:

public abstract class ApplicationSettingProvider : ISettingsProvider
{
    public abstract int GetCurrentSetting();
    public abstract void SetCurrentSetting(int setting);

    public event EventHandler SettingChanged;
    public void OnSettingChanged(int newSetting)
    {
        var handler = SettingChanged;
        if (handler != null)
        {
            handler(this, new SettingChangedEventArgs(newSetting));
        }
    }

The only interface member that's actually "implemented" is.. an event; the other two members are implemented abstract, which means whoever will derive from ApplicationSettingProvider will need to override them anyway: the abstract class is really only there to raise the event.

The naming isn't ideal: the interface is ISettingsProvider, and the implementation is named ApplicationSettingProvider - why did Settings become Setting? Is a Setting the same as an ApplicationSetting? Maybe it's just because it makes things easier to just magically work with Ninject when I use it, but I'd call a class implementing ISettingsProvider, SettingsProvider. Or if the class ought to be ApplicationSettingsProvider, then the interface would be named IApplicationSettingsProvider.

Then there's all the other implementations, all derived from the abstract class; the code that uses these classes should be dependent on the ApplicationSettingProvider abstraction - unless your IoC/DI framework only works with interfaces, that makes ISettingsProvider superfluous.

If the client code is depending on ISettingsProvider, then the abstract class seems like an extraneous level of indirection that's there just so that the OnSettingChanged code isn't repeated in every implementation.

I'd go with the abstract class, be it only to avoid repeating that boilerplate code; if your client code is depending on ApplicationSettingProvider, then I'm pretty sure a static code analysis tool such as ReSharper would have something to say about ISettingsProvider - something like "only implementations of 'GetCurrentSettings' are used" for every single one of its member, clearly indicating that the interface isn't really needed.

It seems to me that the SettingChangedEventArgs is really a PropertyChangedEventArgs in disguise - I'd make ApplicationSettingsProvider implement the existing INotifyPropertyChanged framework-provided interface, and be done with it.

StackExchange Code Review Q#84281, answer score: 5