Global data singleton

Question

I need a global data in my application. My application uses several threads to access and add items to my global variable. The global variable is a ConcurrentDictionary I choose to ensure data is added to the variable. What do you think about my approach?

public class GlobalData
{
    private static volatile GlobalData instance;
    private volatile ConcurrentDictionary<string, object> _data = new ConcurrentDictionary<string, object>();
    private static object syncRoot = new Object();

    public static GlobalData Instance
    {
        get
        {
            if (instance == null)
            {
                lock (syncRoot)
                {
                    if (instance == null)
                        instance = new GlobalData();
                }
            }

            return instance;
        }
    }

    public void Add(string id, object obj)
    {
        _data.TryAdd(id, obj);
    }

    public object Get(string id)
    {
        object value;
        _data.TryGetValue(id, out value);
        return value;
    }
}

Answer 1

• Singletons are evil
• Even if you do want to use it, use Lazy<T> instead of manually doing double-checked locking

Best solution would be to leverage IoC container to inject dependencies you need.

Answer 2

Yes, it can:

1. First, you are mixing statics and non-statics in a confusing way. Please seperate those.
2. A singleton should only be created once. Your class does not have a constructor, so it can be initialized everywhere. Give your class a private constructor. This why, only the class itself can instantiate one.
3. You lock the creation of the GlobalData to prevent problems in a multi threaded environment. Why not put your initialization inside a static constructor or initialize the static field directly, which prevent any problems with multiple threads. This also gets rid of the syncRoot.
4. The keyword volatile is only required on changing values in a multithreaded environment. Your reference to the GlobalData will never change after it has been initialized.

Suggestion:

public class GlobalData
{
    #region Static members

    private static readonly GlobalData instance = new GlobalData();

    public static GlobalData Instance
    {
        get
        {
            return instance;
        }
    }

    #endregion
    #region Instance members

    private readonly ConcurrentDictionary<string, object> _data = new ConcurrentDictionary<string, object>();

    private GlobalData()
    {
    }

    public void Add(string id, object obj)
    {
        _data.TryAdd(id, obj);
    }

    public object Get(string id)
    {
        object value;
        _data.TryGetValue(id, out value);
        return value;
    }

    #endregion
} 

While you are working on it, why not put all code inside a static class, removing the need for one instance and making your code much shorter?

static public class GlobalData
{
    static private readonly ConcurrentDictionary<string, object> _data = new ConcurrentDictionary<string, object>();

    static public void Add(string id, object obj)
    {
        _data.TryAdd(id, obj);
    }

    static public object Get(string id)
    {
        object value;
        _data.TryGetValue(id, out value);
        return value;
    }
} 

Answer 3

I do something like this when I feel like singletons are the answer. I'm not sure if it's considered dependency injection, but it works pretty well for me. You can also swap out functionality easily, by writing a set Dependency function in the class that wants injections then calling it's functions.

public interface IExampleInterface
{
     void Foo(int number);
}

public class OldSingletonClassThatWillBeInjected : IExampleInterface
{
    public OldSingletonClassThatWillBeInjected()
    {

    }

    public void Foo(int value)
    {
       Log(value);
    }
}

public class ClassThatWantsInjection 
{
    IExampleInterface _injection;

    public ClassThatWantsInjection(IExampleInterface injection)
    {
        //Act on this variable through interface methods.
        _injection = injection; 
    }

    public void TestInjection()
    {
        _injection.Foo(5643);
    }
}

public class ProgramMain()
{
    ClassThatWantsInjection _subject;
    OldSingletonClassThatWillBeInjected _oldSingleton;

    static void Main()
    {
        _oldSingleton = new OldSingletonClassThatWillBeInjected();
        //Send the old singleton-y class into the subject.
        _subject = new ClassThatWantsInjection(_oldSingleton);
        _subject.TestInjection();
    }
}

Answer 4

Do not use singleton

If you dare...

... and you are on .NET 4.0 use Lazy<T>