Settings, setting settings, and settings handlers

Question

So, back in this question, I built a set of settings. Now, I am trying to clean the entire system up.

This is the abstract base class:

public class SettingChangedEventArgs : EventArgs
{
 public int NewSetting { get; private set; }
 public SettingChangedEventArgs(int newSetting)
 {
 NewSetting = newSetting;
 }
}
public abstract class ApplicationSettingsProvider
{
 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));
 }
 }
}

Here is a setting built on it:

public class ApplicationThemeProvider : ApplicationSettingsProvider
{
 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();
 }
}

I create an instance of the setting like this:

private static ApplicationSettingsProvider _ThemeProvider = new ApplicationThemeProvider();
private static HomePageVM Data = new HomePageVM(_ThemeProvider);
private static Settings settings = new Settings(_ThemeProvider);
public MainPage()
{
 this.InitializeComponent();
 SettingsPane.GetForCurrentView().CommandsRequested += OnCommandsRequested;
 this.DataContext = Data;
 _ThemeProvider.SettingChanged += (s, e) => { Data.Theme = e.NewSetting; };
}

In the settings SettingsPane:

<StackPanel>
 <TextBlock FontSize="15" Margin="0,5">Text and background color:</TextBlock>
 <ComboBox Name="Theme" SelectionChanged="ThemeChanged">
 <ComboBoxItem Content="Black on White/Purple" Foreground="Black" Background="White"/>
 <ComboBoxItem Content="White on Black/White" Foreground="White" Background="Black"/>
 <ComboBoxItem Content="Black on White/Gold" Foreground="Black" Background="White"/>
 </ComboBox>
</StackPanel>

Which is handled like this:

private ApplicationSettingsProvider _ThemeProvider;
public Settings(ApplicationSettingsProvider themeProvider)
{
 this.InitializeComponent();
 _ThemeProvider = themeProvider;
 InitSettings();
}
private void InitSettings()
{
 Theme.SelectedIndex = _ThemeProvider.GetCurrentSetting();
}
private void ThemeChanged(object sender, SelectionChangedEventArgs e)
{
 _ThemeProvider.SetCurrentSetting(Theme.SelectedIndex);
}

In my VM for the Page:

private ApplicationSettingsProvider _ThemeProvider;
private int _theme = 0;
public int Theme
{
 get { return _theme; }
 set
 {
 if (_theme != value)
 {
 _theme = value;
 OnPropertyChanged();
 }
 }
}
public HomePageVM(ApplicationSettingsProvider themeProvider)
{
 _ThemeProvider = themeProvider;
 Theme = _ThemeProvider.GetCurrentSetting();
}

This is how I am using it in HomePage.xaml:

<ListBox Name="Items" Grid.Column="0" Grid.RowSpan="2" ItemsSource="{Binding ItemList}" DisplayMemberPath="Title" SelectionChanged="OnSelectionChanged"
 Tapped="Items_Tapped" Margin="-2,-2,0,-2" KeyDown="ItemsKeyDown" Style="{Binding Theme, Converter={StaticResource LBStylePick}}"
 ItemContainerStyle="{Binding Theme, Converter={StaticResource LBIStylePick}}" Padding="0,10" SelectedItem="{Binding CurrentItem, Mode=TwoWay}" />

I create my Converters like this:

<local:ThemeToLBStyleConverter x:Key="LBStylePick" />
<local:ThemeToLBIStyleConverter x:Key="LBIStylePick" />

And the C#:

public class ThemeToLBStyleConverter : IValueConverter
{
 public object Convert(object value, Type targetType, object parameter, string language)
 {
 if ((int)value == 2) { return Application.Current.Resources["LBGold"]; }
 else if ((int)value == 1) { return Application.Current.Resources["LBDark"]; }
 else { return Application.Current.Resources["LBLight"]; }
 }
 public object ConvertBack(object value, Type targetType, object parameter, string language)
 {
 return new NotImplementedException();
 }
}
public class ThemeToLBIStyleConverter : IValueConverter
{
 public object Convert(object value, Type targetType, object parameter, string language)
 {
 if ((int)value == 2) { return Application.Current.Resources["LBIGold"]; }
 else if ((int)value == 1) { return Application.Current.Resources["LBIDark"]; }
 else { return Application.Current.Resources["LBILight"]; }
 }
 public object ConvertBack(object value, Type targetType, object parameter, string language)
 {
 return new NotImplementedException();
 }
}

This looks rather complicated, although it is working perfectly. In addition, I don't like how I am handling all the theme values with a plain int, an enum looks to be in order. I am working on changing the ints to enums, but before I adjust every single thing that uses it, I would like to know if I should change this process in any way so all my work doesn't go to waste.

My other two settings can be viewed on PasteBin in the provided link.

• Heslacher: Can you either post a link to the remaining setting clases or add one ore two more to your question ? I will take a look at it tomorrow if it is still needed then.

Answer 1

Your settings are very similiar and differ only in the name of the setting and the way you are retrieving the setting. This screams for injecting the setting name in the constructor in the baseclass and adding an interface for getting the values.

This could look like

public interface IIntegerSettingProvider
{
 int GetCurrentSetting();
} 
public abstract class ApplicationSettingsProvider : IIntegerSettingProvider
{
 protected readonly string settingName;
 public ApplicationSettingsProvider(string settingName)
 {
 this.settingName = settingName;
 ApplicationData.Current.DataChanged += (a, o) =>
 {
 CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
 {
 OnSettingChanged((int)ApplicationData.Current.RoamingSettings.Values[settingName]);
 });
 };
 }
 public void SetCurrentSetting(int value)
 {
 ApplicationData.Current.RoamingSettings.Values[settingName] = value;
 ApplicationData.Current.SignalDataChanged();
 }
 public event EventHandler<SettingChangedEventArgs> SettingChanged;
 public void OnSettingChanged(int newSetting)
 {
 var handler = SettingChanged;
 if (handler != null)
 {
 handler(this, new SettingChangedEventArgs(newSetting));
 }
 }
}

The logic itself looks a little strange to me, although I don't know anything about windows phone development.

If a value is set by calling SetCurrentSetting() you are signaling a ApplicationData.Current.SignalDataChanged(); to which you had added a handler which then calls OnSettingChanged() which should by the way be protected instead of public.

Wouldn't it be easier to just call OnSettingChanged() from inside the SetCurrentSetting() method like:

public abstract class ApplicationSettingsProvider : IIntegerSettingProvider
{
 private readonly string settingName;
 public ApplicationSettingsProvider(string settingName)
 {
 this.settingName = settingName;
 }
 public void SetCurrentSetting(int value)
 {
 OnSettingChanged(value);
 }
 public event EventHandler<SettingChangedEventArgs> SettingChanged;
 protected void OnSettingChanged(int newSetting)
 {
 var handler = SettingChanged;
 if (handler != null)
 {
 handler(this, new SettingChangedEventArgs(newSetting));
 }
 }
} 

This would lead for the ApplicationThemeProvider to

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

The only thing you would need to change in the classes which consumes the setting provider would be a cast of the provider to an IIntegerSettingProvider like

private ApplicationSettingsProvider _ThemeProvider;
public Settings(ApplicationSettingsProvider themeProvider)
{
 this.InitializeComponent();
 _ThemeProvider = themeProvider;
 InitSettings();
}
private void InitSettings()
{
 Theme.SelectedIndex = ((IIntegerSettingProvider)_ThemeProvider).GetCurrentSetting();
}
private void ThemeChanged(object sender, SelectionChangedEventArgs e)
{
 _ThemeProvider.SetCurrentSetting(Theme.SelectedIndex);
}

but hey we could do better than this. Why shouldn't we add the SetCurrentSetting() method and the SettingChanged eventhandler to the interface. If we do this we will use the interface here which is almost always better than coding against an implementation.

The interface would look like

public interface IIntegerSettingProvider
{
 int GetCurrentSetting();
 void SetCurrentSetting(int value);
 public event EventHandler<SettingChangedEventArgs> SettingChanged;
} 

the ApplicationSettingsProviderfrom above can stay like it is.

The usage would then look like

private static IIntegerSettingProvider _ThemeProvider = new ApplicationThemeProvider();
private static HomePageVM Data = new HomePageVM(_ThemeProvider);
private static Settings settings = new Settings(_ThemeProvider);
public MainPage()
{
 this.InitializeComponent();
 SettingsPane.GetForCurrentView().CommandsRequested += OnCommandsRequested;
 this.DataContext = Data;
 _ThemeProvider.SettingChanged += (s, e) => { Data.Theme = e.NewSetting; };
} 

and the handling like

private IIntegerSettingProvider _ThemeProvider;
public Settings(IIntegerSettingProvider themeProvider)
{
 this.InitializeComponent();
 _ThemeProvider = themeProvider;
 InitSettings();
}
private void InitSettings()
{
 Theme.SelectedIndex = _ThemeProvider.GetCurrentSetting();
}
private void ThemeChanged(object sender, SelectionChangedEventArgs e)
{
 _ThemeProvider.SetCurrentSetting(Theme.SelectedIndex);
} 

---

This

public object Convert(object value, Type targetType, object parameter, string language)
{
 if ((int)value == 2) { return Application.Current.Resources["LBGold"]; }
 else if ((int)value == 1) { return Application.Current.Resources["LBDark"]; }
 else { return Application.Current.Resources["LBLight"]; }
} 

does not look readable and can be improved by boxing object value only once and using some indentation like

private const string defaultResourceName = "LBLight";
public object Convert(object value, Type targetType, object parameter, string language)
{
 int current = (int)value;
 string resourceName = defaultResourceName;
 if (current == 2) 
 {
 resourceName = "LBGold"; 
 }
 else if (current == 1) 
 { 
 resourceName = "LBDark"; 
 }
 return Application.Current.Resources[resourceName];
} 

Stuffing single instructions on the same line than the ifcondition can IMHO be done only for a guard clause (separated by a new line) but doing this with an else ifand an else also reduces the readability to a minimum.

• BCdotWEB: WRT the last part of your review (public object Convert): perhaps a switch could be better? Also, instead of doing return Application.Current.Resources[some_string] three times, perhaps the switch could simply set the value of some_string and then there'd be only one call to return Application.Current.Resources[some_string]?
• Heslacher: @BCdotWEB good catch about the returns. A switch for only 2 conditions would be too much IMHO. Updated answer.

Answer 2

• ThemeToLBStyleConverter and ThemeToLBIStyleConverter have identical code, save for the strings in the square brackets. They should both be subclasses of something else, with the strings as members.
• In ApplicationThemeProvider, you should use .TryGetValue() rather than .ContainsKey() and a lookup.

• BCdotWEB: Upvote for TryGetValue() in particular. Disappointingly Microsoft's own example code usually fails to use this method.