Problem
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<SettingChangedEventArgs> SettingChanged;
}
public abstract class ApplicationSettingProvider : ISettingsProvider
{
public abstract int GetCurrentSetting();
public abstract void SetCurrentSetting(int setting);
public event EventHandler<SettingChangedEventArgs> 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;
ApplicationData.Current.SignalDataChanged();
}
}
ApplicationFontSizeProvider:
public class ApplicationFontSizeProvider : ApplicationSettingProvider
{
public ApplicationFontSizeProvider()
{
ApplicationData.Current.DataChanged += (a, o) =>
{
CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
{
OnSettingChanged((int)ApplicationData.Current.RoamingSettings.Values["SetFontSize"]);
});
};
}
public override int GetCurrentSetting()
{
if (ApplicationData.Current.RoamingSettings.Values.ContainsKey("SetFontSize"))
{
int value = (int)ApplicationData.Current.RoamingSettings.Values["SetFontSize"];
if (value == 0) { return 15; }
else if (value == 1) { return 20; }
else if (value == 2) { return 30; }
else if (value == 15 || value == 20 || value == 30) { return value; }
else { return 20; }
}
return 20;
}
public override void SetCurrentSetting(int fontSize)
{
ApplicationData.Current.RoamingSettings.Values["SetFontSize"] = fontSize;
ApplicationData.Current.SignalDataChanged();
}
}
ApplicationStartupVersionProvider:
public class ApplicationStartupVersionProvider : ApplicationSettingProvider
{
public ApplicationStartupVersionProvider()
{
ApplicationData.Current.DataChanged += (a, o) =>
{
CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
{
OnSettingChanged((int)ApplicationData.Current.RoamingSettings.Values["StartupVersion"]);
});
};
}
public override int GetCurrentSetting()
{
bool WindowsStore = true;
#if WINDOWS_PHONE_APP
WindowsStore = false;
#endif
if (ApplicationData.Current.RoamingSettings.Values.ContainsKey("StartupVersion"))
{
int value = (int)ApplicationData.Current.RoamingSettings.Values["StartupVersion"];
if (value >= 0 && value <= 2) { return value; }
else { return WindowsStore ? 0 : 1; }
}
else { return WindowsStore ? 0 : 1; }
}
public override void SetCurrentSetting(int version)
{
ApplicationData.Current.RoamingSettings.Values["StartupVersion"] = version;
ApplicationData.Current.SignalDataChanged();
}
}
How else should I improve this code?
Solution
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<SettingChangedEventArgs> 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<SettingChangedEventArgs> 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.
I’ll start by agreeing with Mug that a ISettingsProvider should have a CurrentSetting property and simply also implement INotifyPropertyChanged.
ApplicationData.Current.DataChanged += (a, o) => ...
I like delegate expressions as much as the next guy, but it’s not always appropriate to use one letter identifiers in them. I mean, we complain when people use one letter variables elsewhere, why should we treat lambda expressions any differently? If they’re not immediately obvious, we should give them a proper name.
I’m not a huge fan of single lining if statements.
if (value == 0) { return 15; }
else if (value == 1) { return 20; }
else if (value == 2) { return 30; }
else if (value == 15 || value == 20 || value == 30) { return value; }
else { return 20; }
But, I think you made a wise decision to do so here. It reads very nicely. I think expanding the braces to newlines would have hurt a bit. What are all these magic numbers though? What do they mean? It could certainly use some constants.
I keep coming back to this code in your constructors.
ApplicationData.Current.DataChanged += (a, o) =>
{
CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
{
OnSettingChanged((int)ApplicationData.Current.RoamingSettings.Values["StartupVersion"]);
});
};
Again, I like delegates as much as the next guy, but only when they’re buying me something. The only difference between the different delegates is the string key that you’re passing in. I’d create a protected virtual (and generic) method in your abstract base class that takes in the string key.
protected virtual void RaiseSettingChanged<TSetting>(string settingKey)
{
CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
{
OnSettingChanged((TSetting)ApplicationData.Current.RoamingSettings.Values[settingKey]);
});
}
Then back in your constructors, the delegate gets wired up a bit more simply.
ApplicationData.Current.DataChanged += (a, o) => RaiseSettingChanged<int>("StartupVersion");
I recommend making it virtual so you can override the behavior if you find a need. Perhaps it’s premature to do so.
Signify unused by _
As a widespread convention, unused variables are named _ so in your delegate:
ApplicationData.Current.DataChanged += (a, o) =>
{
CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
{
OnSettingChanged((int)ApplicationData.Current.RoamingSettings.Values["StartupVersion"]);
});
};
You may explicitly state that you will not use a nor b with:
ApplicationData.Current.DataChanged += (_, _) =>