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?

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...

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.

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).