1
\$\begingroup\$

I've taken over a code base that is still very much in it's infancy, and stumbled across this class. I'm just wondering if it's actually worth it to have a SingletonProvider, and making use of Lazy<T> if it's just going to return the value straight away?

public static class SingletonProvider
{
    private static readonly ConcurrentDictionary<Type, Lazy<object>> Instances = new ConcurrentDictionary<Type, Lazy<object>>();

    public static T GetSingleton<T>()
        where T : class, new()
    {
        return Instances.GetOrAdd(typeof(T), t => new Lazy<object>(() => Activator.CreateInstance(t), isThreadSafe: true)).Value as T;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Doesn't make much sense that singleton T must have a default parameterless constructor does it? \$\endgroup\$ – Mathieu Guindon Sep 20 '16 at 13:45
  • \$\begingroup\$ @Mat'sMug Another good point! I feel, if one is going to make a class a singleton, then do so explicitly. \$\endgroup\$ – Richard Ueckermann Sep 20 '16 at 13:49
  • 1
    \$\begingroup\$ Well T is not, by definition, a singleton; anyone and anything is free to instantiate it at will. This provider seems like a bad idea... indeed badly implemented ;-) \$\endgroup\$ – Mathieu Guindon Sep 20 '16 at 13:51
5
\$\begingroup\$

I hope that code base isn't sprinkled with calls to that SingletonProvider!

A "singleton provider", conceptually, makes no sense. This is highlighted by this generic type constraint:

where T : class, new()

By definition, T cannot be a singleton - it must expose a public, parameterless constructor to satisfy the type constraint!

I'm not sure what to call this provider, but it definitely needs a rename refactoring, to remove the confusing notion of "Singleton".

Consider this hypothetical client code:

var foo = SingletonProvider.GetSingleton<Something>();

And then, elsewhere:

var bar = SingletonProvider.GetSingleton<Something>();

What have we gained? foo is a reference to the same object as bar. But if Something must be a Singleton, then a much less surprising approach would be something like:

var foo = Something.GetInstance();

And then, there's no way to accidentally do this elsewhere:

var bar = new Something();

...and you keep your code coupled with one class instead of two.


I see what problem this class is trying to solve: it's not all that rare that an object must only ever exist in once instance.

The problem it's creating is that it's imposing way too heavy constraints on the type (T), and as a result this Something class must in turn be either tightly coupled with its own dependencies, or have them property-injected... which is unnecessary complexity.

I'd much rather code against an abstraction, say ISomething, and let the Something implementation decide how it wants to deal with its dependencies:

public class Something : ISomething
{
    private readonly IFoo _foo;
    private readonly IBar _bar;

    public Something(IFoo foo, IBar bar)
    {
        _foo = foo;
        _bar = bar;
    }

    //...
}

If I have types that must depend on ISomething, I'll have them receive an instance in their constructor, have my favorite IoC container inject all its dependencies, and if I instruct my IoC container that whoever requires an ISomething implementation should always receive the same Something instance, I can do that without even implementing a Singleton - e.g. with Ninject it would look something like this:

kernel.Bind<ISomething>().To<Something>().InSingletonScope();

The takeaway here is that the responsibility of creating objects and managing object lifetimes is the responsibility of the IoC container - not of some "SingletonProvider".


So why all this talk about IoC and dependency injection? Because good, SOLID object-oriented code strives for low coupling (and high cohesion too). By coding against abstractions, you effectively reduce coupling to concrete types to a minimum; by using this SingletonProvider, you're not only coupled with the SingletonProvider class, but also with the concrete implementation of whatever the type of T is.

That SingletonProvider seems like a good way to make testing very hard, for very little benefit. It forces your client code to depend on concrete types, which increases coupling.


As for the implementation itself, I agree with you - it makes no sense to use a Lazy<T> if you're going to retrieve its value in the very same instruction; that leaves you with your already-initialized Lazy<T> instance in the dictionary, so you might as well directly store the instance itself.

| improve this answer | |
\$\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.