3
\$\begingroup\$

I want to store some user settings into a file, so that the users get the same experience on all machines they're working on. The built-in user.config turned out to be insufficient, as I can't store Nullable<T> values in it.

I created a simple class for my program that stores the user settings in an XML file. This is how the file may look like:

<?xml version="1.0" encoding="utf-16"?>
<UserSettings>
    <Section1>
        <SubSectionA>
            <ASetting1>value</ASetting1>
            <ASetting2>value</ASetting2>
            <ASetting3>value</ASetting3>
        </SubSectionA>
        <SubSectionB>
            <BSetting1>value</BSetting1>
        </SubSectionB>
        <SubSectionC>
            <CSetting1>value</CSetting1>
            <CSetting2>value</CSetting2>
        </SubSectionC>
    </Section1>
    <Section2>
        <SubSectionA>
            <ASetting1>value</ASetting1>
            <ASetting2>value</ASetting2>
            <ASetting3>value</ASetting3>
        </SubSectionA>
        <SubSectionB>
            <BSetting1>value</BSetting1>
        </SubSectionB>
        <SubSectionC>
            <CSetting1>value</CSetting1>
            <CSetting2>value</CSetting2>
        </SubSectionC>
    </Section2>
    <Section3>
        ...
    </Section3>
</UserSettings>

Finally, this is my UserSettings class:

public class UserSettings
{
    static UserSettings instance;

    private static UserSettings Instance
    {
        get
        {
            if (instance == null)
                instance = new UserSettings();
            return instance;
        }
    }

    // This is a sample setting
    public static DateTime? Section1_SubSectionA_ASetting1
    {
        get
        {
            DateTime storedValue;
            if (DateTime.TryParse(Instance.GetSetting("Section1", "SubSectionA", "ASetting1"), out storedValue))
                return storedValue;
            else
                // Return the default application value for this setting
                return null;
        }
        set { Instance.SaveSetting(value, "Section1", "SubSectionA", "ASetting1"); }
    }

    // Static
    // ---------------------------------------------------------------------
    // Instance

    readonly string settingsFilePath;
    XDocument settingsFile;

    private UserSettings()
    {
        this.settingsFilePath = Environment.GetFolderPath(Environment.SpecialFolder.ApplicationData) + @"\MyCompany\MyApplication\UserSettings.xml";

        // Create the settings file if neccessary
        if (!File.Exists(this.settingsFilePath))
        {
            XDocument newSettingsFile = new XDocument(
                new XDeclaration("1.0", "utf-16", "true"),
                new XElement("UserSettings"));

            // Create the folders if they not exist
            new FileInfo(this.settingsFilePath).Directory.Create();

            newSettingsFile.Save(this.settingsFilePath);
            this.settingsFile = newSettingsFile;
        }
        else
            this.settingsFile = XDocument.Load(this.settingsFilePath);
    }

    private string GetSetting(params string[] elementNames)
    {
        XElement element = this.settingsFile.Root;

        // Walk through the given element names
        foreach (string elementName in elementNames)
        {
            XElement child = element.Elements(elementName).FirstOrDefault();

            // If an element does not exist, the setting has not been set yet, return the default value
            if (child == null)
                return default(string);

            element = child;
        }
        return element.Value;
    }

    private void SaveSetting(object value, params string[] elementNames)
    {
        XElement element = this.settingsFile.Root;
        foreach (string elementName in elementNames)
        {
            XElement child = element.Elements(elementName).FirstOrDefault();

            // If an element does not exist, the setting has not been set yet, create the element and move on
            if (child == null)
            {
                child = new XElement(elementName);
                element.Add(child);
            }

            element = child;
        }
        element.Value = value.ToString();

        // Save the file to disk
        this.settingsFile.Save(this.settingsFilePath);
    }
}

Is there any way I can improve my code? You can assume that there are no sibling XML elements that share the same name. If so, the code should always read from and write to the first one, ignoring the others.

\$\endgroup\$
3
  • \$\begingroup\$ I just noticed that I should add some code to catch exceptions on reading and writing the config file (missing permissions, file is read-only, ...). \$\endgroup\$ Aug 28, 2015 at 8:59
  • \$\begingroup\$ What are you expecting to be returned from GetSetting()? \$\endgroup\$
    – dreza
    Aug 28, 2015 at 19:35
  • \$\begingroup\$ I don't think nullables in settings do any good. Settings should always provide some value because you have to fallback to something anyway... besides if you are looking for an easy settings framework (or inspiration) you might check-out my SmartConfig ;-] (link in my profile) \$\endgroup\$
    – t3chb0t
    Aug 29, 2015 at 22:02

1 Answer 1

2
\$\begingroup\$

I see a few things that you could improve. I also consider it as an exercise for reading xml files so I won't suggest you to use a xml serializer to directly turn everything into an object.

  • You shouldn't call other methods as parameters not only because the call is extremely long but it's also easier to debug it later if you can see the result of that call:

so instead of

if (DateTime.TryParse(Instance.GetSetting("Section1", "SubSectionA", "ASetting1"), out storedValue))

this would be better

var setting = Instance.GetSetting("Section1", "SubSectionA", "ASetting1");
if (DateTime.TryParse(setting, out storedValue))
  • The GetSetting method you can make shorter by using XPath

It's just an example, you should build the path dynamicly based on params string[] elementNames

var xPath = "//UserSettings/Section1/SubSectionA/ASetting1";
var xSetting = xConfig.XPathSelectElement(xPath);
  • Both methods GetSetting and SaveSetting use almost the same code, it looks like Copy&Paste. You should create a single method for getting a setting element to get rid of the repetitions like XElement GetElement(params string[] elementNames)

  • This is not a good name for a method Section1_SubSectionA_ASetting1, give it a more meaningful name like GetBirthdate

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.