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.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Aug 28, 2015 at 8:52
\$\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\$ Commented Aug 28, 2015 at 8:59
  • \$\begingroup\$ What are you expecting to be returned from GetSetting()? \$\endgroup\$ Commented 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\$ Commented 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

answered Aug 30, 2015 at 8:14
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.