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;
 }
}

• abuzittin gillifirca: Why do you think you need shared data? What are the threads doing with it? You have separate Add and Get methods, which means, dollars to dimes, you have a race condition.
• dreza: Read this for a recommended approach for creating singletons codereview.stackexchange.com/questions/79/…

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;
 }
} 

• Luke101: I like the second version. Can threads add data to the dictionary and retrieve the data at anytime. I see that the _data variable is read-only does that mean i can only add a value to it one?
• Martin Mulder: @Luke: The keyword readonly only says something about the reference to the object. It does not say anything about the values inside the object. In this case, when you have created a new dictionary, that reference cannot be modified (outside the constructor). But the contents of the dictionary can be modified.
• Martin Mulder: @Luke: The dictionary is a ConcurrentDictionary which means that the dictionary will synchronize all multithreaded operations. In this case, multiple readers are allowed, and one writer is allowed. Reads and writes cannot be combined, but the dictionary will manage that himself. Do not worry about that.
• Mathieu Guindon: The access modifier comes first, it's public static class GlobalData - nice review nonetheless, +1 :)

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>