Settings class in C# application

Question

I made a settings class for my own C# application. My main goal was to reach this class from every other class (this is why I choose the Singleton pattern). I also wanted to distinguish the different type of settings, so I added GeneralSettings and made it public. Do you know any other ways which I can tell apart the different type of settings? I thought about namespaces, but those are not allowed in classes.

I also wanted to make sure that I can restore its state if the user don't want to save the changes made in the settings dialog. What is your opinion about my implementation? Now I need to call SetRestorePoint and if I want to discard changes Restore.

public class Settings : ISerializable
{
 private Settings ()
 {
 }
 static private Settings _instance = null;
 static private Settings _restoreInstance = null;
 static public Settings Get //Singleton
 {
 get
 {
 if (_instance == null)
 _instance = new Settings ();
 return _instance;
 }
 private set { }
 }
 public void SetRestorePoint ()
 {
 _restoreInstance = DeepClone (_instance);
 }
 public Settings Restore ()
 {
 _instance = _restoreInstance;
 return _instance;
 }
 public GeneralSettings General = new GeneralSettings ();
 [Serializable] 
 public class GeneralSettings
 {
 public bool isAutomaticSave = false;
 }
 private static T DeepClone<T>(T obj)
 {
 using (var ms = new System.IO.MemoryStream ())
 {
 var formatter = new System.Runtime.Serialization.Formatters.Binary.BinaryFormatter ();
 formatter.Serialize (ms, obj);
 ms.Position = 0;
 return (T) formatter.Deserialize (ms); 
 }
 }
 //serialization...
}

How have people implemented Settings in bigger software?

• t3chb0t: Well, I use a static class and properties and preload the settings.
• Mathieu Guindon: Why not include the ISerializable implementation?
• Der Kommissar: What type of project/application is this? Why not use the built-in Settings functionality?
• t3chb0t: @EBrown probably because it's one of the worst things ms has ever created?

Answer 1

The indentation isn't always consistent, e.g. the using scope in DeepClone should be indented a level.

Get property should probably be named Instance (a noun), or remain Get (a verb) and be turned into a method. The private and not implemented setter is sloppy, and the = null initialization of the backing private field is redundant.

Seems the restore mechanics are written with a specific UI in mind, where the UI immediately saves changes - typically a settings dialog would do something when the user confirms or applies modifications; seems your model is the other way around - the changes are saved unless they're cancelled, so cancelling changes requires work... seems backwards.

I'd remove that cloning and restore functionality, and make the UI work differently, off a ViewModel - not directly with the settings; when the dialog is cancelled, you have nothing to revert or restore (no changes were made), and when the dialog is okayed and/or changes are applied, then you persist the changes using your settings service.

I think a Singleton isn't needed for this, provided that the rest of the code isn't responsible for newing up anything; if you make that responsibility the job of an IoC container, then it's child's play to configure it in such a way that any class with an ISettingsService constructor parameter will always receive the same instance of whatever concrete type implements that interface - that's OOP with Dependency Injection at play, and static members don't play very well with OOP (static members belong to the type, not to an object), so the more of it you have in your code base, the more coupling you have everywhere between unrelated classes...

• Ryper: Thank you very much for your suggestions. You are right, Instance is much better. This is an awesome Idea, I will remove the Restore function, and only make the changes when the user clicks on the Accept button. So you suggest me to add Settings to the objects in constructors? What happens if the settings are changed and the objects are already constructed?

Answer 2

In addition to what other reviewers mentioned

• The Settings class isn't really Singleton as the class isn't marked with sealed keyword. This defeats the idea of Singleton which prevents subclassing. Marking the class with sealed helps the JIT(Just In Time) compiler to optimise things more but it's not a tight constraint to be honest. You can read more about that here Implementing the Singleton Pattern in C#

• If this code is meant to be used by an application, which I presume it is. Jon Skeet explains in his article that

  Two different threads could both have evaluated the test if (instance==null) and found it to be true, then both create instances, which violates the singleton pattern

  We can refactor your code to

 public sealed class Settings:ISerializable
 {
 private Settings ()
 {
 }
 static private Settings _instance = null;
 static private Settings _restoreInstance = null;
 private static readonly object padlock = new object();
 static public Settings Get //Singleton
 {
 get
 {
 lock (padlock)
 {
 if (_instance == null)
 {
 _instance = new Settings ();
 }
 return _instance;
 }
 }
 //private set { }
 }
 }

P.S If performance is not a priority, using a lock(object) can achieve significant result and avoid several instances. Good news is there are also some lazy implementations that avoids using lock. read more from the link provided.

Should you be changing the state of the Instance using SetRestorePoint()?

NO, this defeats the point of using the Singleton pattern. If you want to achieve changing the state and having a single instance then you should consider using the factory pattern.

Should you be calling the Serialize and Deserialize without a try .. catch or a using statement ?

No, SerializationException can be thrown within the DeepClone method. I will let you figure this out .

I hope this helps cheers.

• JanDotNet: The first point isn't completely valid because the private constructor prevents the Settings class from being sub classed ;). Nevertheless, using the sealed is a good idea :).
• Tolani: @JanDotNet yeah your right - The class can't be extended with sealed but instantiating the class is impossible with private.Nevertheless, it can still be extended. I will make amends to my answer

Answer 3

Code Style

• The empty setter is unnecessary
• The field General should be a read-only property
• The fiels isAutomaticSave should be a property

Logic

The object is not a real singelton because the method Restore changes the internal instance. That may lead to strange errors (imagine the case that the settings object was bound to the GUI which is generally possible with singletons).

• radarbob: The implicit default constructor is public. Any constructor overrides would hide the default, but there are none here. So to make the default constructor private it must be written explicitly. However, once written it is private by default. so Settings() { } (no access modifier) is private.
• JanDotNet: Thanks... I overlooked the private modifier. The private constructore is required for singletons of corse. I removed the point "The empty constructor is unnecessary" from my answer.