c# - How to improve the initialization of a Settings window? - Stack Overflow

I have a class like this:

public static class Settings
{
    private static SettingsWindow _settingsWindow = null;

    public static void Init(SettingsWindow settingsWindow)
    {
        _settingsWindow = settingsWindow;
    }

Then later is the following (inside a method):

_settingsWindow.ExePathTextBox.Text = CurrentSettings.ExePath;
_settingsWindow.rbSimpleSave.Checked = !CurrentSettings.optSaveUseBrowser;
_settingsWindow.rbBrowserSave.Checked = CurrentSettings.optSaveUseBrowser;
_settingsWindow.rbPresetAll.Checked = CurrentSettings.PreventPresetCol == PC.Allow;
_settingsWindow.rbPresetFolder.Checked = CurrentSettings.PreventPresetCol == PC.Folder;
_settingsWindow.rbPresetGlobal.Checked = CurrentSettings.PreventPresetCol == PC.Global;
_settingsWindow.rbFolderAll.Checked = CurrentSettings.PreventFolderCol == PC.Allow;
_settingsWindow.rbFolderParent.Checked = CurrentSettings.PreventFolderCol == PC.Folder;
_settingsWindow.rbFolderGlobal.Checked = CurrentSettings.PreventFolderCol == PC.Global;

Is there some way to shortcut these statements to avoid repeating _settingsWindow on each line?

I hope there exists some obvious answer which I'm somehow not finding.

I have a class like this:

public static class Settings
{
    private static SettingsWindow _settingsWindow = null;

    public static void Init(SettingsWindow settingsWindow)
    {
        _settingsWindow = settingsWindow;
    }

Then later is the following (inside a method):

_settingsWindow.ExePathTextBox.Text = CurrentSettings.ExePath;
_settingsWindow.rbSimpleSave.Checked = !CurrentSettings.optSaveUseBrowser;
_settingsWindow.rbBrowserSave.Checked = CurrentSettings.optSaveUseBrowser;
_settingsWindow.rbPresetAll.Checked = CurrentSettings.PreventPresetCol == PC.Allow;
_settingsWindow.rbPresetFolder.Checked = CurrentSettings.PreventPresetCol == PC.Folder;
_settingsWindow.rbPresetGlobal.Checked = CurrentSettings.PreventPresetCol == PC.Global;
_settingsWindow.rbFolderAll.Checked = CurrentSettings.PreventFolderCol == PC.Allow;
_settingsWindow.rbFolderParent.Checked = CurrentSettings.PreventFolderCol == PC.Folder;
_settingsWindow.rbFolderGlobal.Checked = CurrentSettings.PreventFolderCol == PC.Global;

Is there some way to shortcut these statements to avoid repeating _settingsWindow on each line?

I hope there exists some obvious answer which I'm somehow not finding.

• c#
• winforms
• class
• static

Share Improve this question Follow edited Jan 31 at 13:10 Steve Stover asked Jan 29 at 3:50 Steve StoverSteve Stover 6722 silver badges1111 bronze badges 10

• Switch to VB.Net and use With – Yuriy Faktorovich Commented Jan 29 at 4:22
• Well that may be the obvious thing to do (VB.Net) but it is not desirable in this case... @Yuriy – Steve Stover Commented Jan 29 at 4:33
• 2 var s = _settingsWindow which also makes sure you're accessing the same instance in an almost thread safe way. – Jeremy Lakeman Commented Jan 29 at 4:44
• @Jeremy since _settingsWindow is static this shouldn't penalize me memory-wise right? Just a pointer if you will, to the same instance? – Steve Stover Commented Jan 29 at 5:03
• Why do you not set the properties, when you create SettingsWindow? – Jonathan Willcock Commented Jan 29 at 7:20

 |  Show 5 more comments

2 Answers 2

Sorted by: Reset to default 1

IMO, settings the values of form elements is the responsibility of the form itself, not some outside class.
Therefor, you'd be much better off creating a method in the SettingsWindow class that takes in an instance of (what I'm guessing is) Settings and use that to populate the form:

// in SettingsWindow
public void Initialize(Settings currentSettings)
{
    ExePathTextBox.Text = currentSettings.ExePath;
    rbSimpleSave.Checked = !CurrentSettings.optSaveUseBrowser;
    // do the same for the rest of the form elements 
}

and then, you can simply call this method from your Settings class:

// in Settings class

public static void InitializeForm() => _settingsWindow.Initialize(CurrentSettings);

In your comment, you stated:

I'm not the original author

And looking at that copied code, we might ask: What were they thinking?

I mean that literally. It is plausible, fair and reasonable to posit that they might have had two idiomatic goals that are common in this scenario:

---

• First: The misuse of a static class might have come from an objective of making the settings accessible globally from anywhere in the app. If this were the case, however, it's far better to make a static instance of a non-static class.

public partial class MainForm : Form
{
    // The AppSettings class is 'not' static, but the instance is.
    public static AppSettings CurrentSettings { get; private set; } =
        new AppSettings();

    // In this particular case, we're serializing to a json file.
    public string PathToSettings { get; } =
        Path.Combine(
            Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
            "StackOverflow",
            "SettingsDemo",
            "config.json");

    public MainForm()
    {
        if (File.Exists(PathToSettings))
        {
            CurrentSettings =
                JsonConvert
                .DeserializeObject<AppSettings>(File.ReadAllText(PathToSettings));
        }
        else
        {
            Directory.CreateDirectory(Path.GetDirectoryName(PathToSettings));
            File.WriteAllText(PathToSettings, JsonConvert.SerializeObject(CurrentSettings));
        }
        InitializeComponent();
        toolStrip.DataBindings.Add(
            nameof(toolStrip.BackColor),
            CurrentSettings,
            nameof(CurrentSettings.ToolStripBackColor),
            false,
            DataSourceUpdateMode.OnPropertyChanged);
        this.settingsButton.Click += SettingsButton_Click;
    }
}

• Second: It's quite common to provide a chance to bail out when editing properties. So, suppose we have a PropertyGrid set up for our AppSettings class. Since the options here are to [Apply] or [Cancel], we need to work with a copy of the settings (not the settings themselves). I believe that this need for a 'revert' is responsible for the tortured manner that the copying was done in the original code.

So, here's the thing:

The PropertyGrid already uses reflection to populate itself, and we could employ the same strategy to make the copy. Here, we have added a CopyValues() method that reflects the public properties.

public class AppSettings : INotifyPropertyChanged
{
    public void CopyValues(AppSettings target)
    {
        foreach (var property in typeof(AppSettings).GetProperties())
        {
            property.SetValue(target, property.GetValue(this));
        }
    }
    .
    .
    .
    // Property declarations...
}

---

Minimal Example

Here, the SettingsWindow is a simple Form containing a PropertyGrid.

public partial class SettingsWindow : Form
{
    public AppSettings EditedSettings { get; } = new AppSettings();
    public SettingsWindow()
    {
        InitializeComponent();
        buttonApply.Click += (sender, e) =>DialogResult = DialogResult.OK;
        buttonCancel.Click += (sender, e) =>DialogResult = DialogResult.Cancel;
        propertyGrid.SelectedObject = EditedSettings;
    }
    public DialogResult ShowDialog(IWin32Window owner, AppSettings currentSettings)
    {
        currentSettings.CopyValues(EditedSettings);
        return base.ShowDialog(owner);
    }
}

---

Call from MainForm Gear Icon

Call its custom ShowDialog overload, passing the CurrentAppSettings and updating them only if the method returns DialogResult.OK.

public partial class MainForm : Form
{
    .
    .
    .

    private void SettingsButton_Click(object? sender, EventArgs e)
    {
        using(var settingsUI = new SettingsWindow())
        {
            if(DialogResult.OK == settingsUI.ShowDialog(this, CurrentSettings))
            {
                settingsUI.EditedSettings.CopyValues(CurrentSettings);
                File.WriteAllText(PathToSettings, JsonConvert.SerializeObject(CurrentSettings));
            }
            else
            {   /* G T K */
                // User has decided not to change the values after all.
            }
        }
    }
}