3
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – abuzittin gillifirca May 22 '13 at 14:51
  • \$\begingroup\$ Read this for a recommended approach for creating singletons codereview.stackexchange.com/questions/79/… \$\endgroup\$ – dreza May 22 '13 at 20:16
7
\$\begingroup\$
  • 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.

\$\endgroup\$
5
\$\begingroup\$

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;
    }
} 
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Luke101 May 22 '13 at 15:02
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Martin Mulder May 22 '13 at 15:39
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Martin Mulder May 22 '13 at 15:40
  • \$\begingroup\$ The access modifier comes first, it's public static class GlobalData - nice review nonetheless, +1 :) \$\endgroup\$ – Mathieu Guindon May 9 '14 at 23:30
2
\$\begingroup\$

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();
    }
}
\$\endgroup\$
-2
\$\begingroup\$

Do not use singleton

If you dare...

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.