2
\$\begingroup\$

I have written a wrapper around the Microsoft's Unity container and would like this to be cod reviewed please. Here is the interface definition:

/// <summary>
/// A wrapper around the Inversion of Control container.
/// </summary>
public interface IIoCWrapper
{
    #region Methods

    /// <summary>
    /// Registers an instance of given type. This is particularly useful when registering a mocked instance.
    /// </summary>
    /// <typeparam name="TInterface">Type of the instance to register.</typeparam>
    /// <param name="instance">The instance to register.</param>
    void RegisterInstance<TInterface>(TInterface instance);

    /// <summary>
    /// Registers a type with a name associated with it.
    /// </summary>
    /// <typeparam name="TInterface">Interface that is implemented by the type.</typeparam>
    /// <typeparam name="TImplementation">Type that implements the interface.</typeparam>
    /// <param name="name">Name of the type or an identifier that can be used when resolving it.</param>
    void RegisterNamedType<TInterface, TImplementation>(string name)
        where TInterface : class where TImplementation : class, TInterface;

    /// <summary>
    /// Registers a type implementing an interface.
    /// </summary>
    /// <typeparam name="TInterface">Interface that is implemented by the type.</typeparam>
    /// <typeparam name="TImplementation">Type that implements the interface.</typeparam>
    void RegisterType<TInterface, TImplementation>()
        where TInterface : class where TImplementation : class, TInterface;

    /// <summary>
    /// Resolves the type parameter T to an instance of the appropriate named type.
    /// </summary>
    /// <typeparam name="T">Type of object to return</typeparam>
    /// <param name="name">Name of the type that was used at the time of named registration.</param>
    /// <returns>Object of type T.</returns>
    T ResolveNamedType<T>(string name);

    /// <summary>
    /// Resolves the type parameter T to an instance of the appropriate type.
    /// </summary>
    /// <typeparam name="T">Type of object to return</typeparam>
    T ResolveType<T>();

    #endregion
}

Its implementation:

/// <summary>
    /// A wrapper around the Inversion of Control container.
    /// </summary>
    public class IoCWrapper : IIoCWrapper
    {
        #region Fields

        /// <summary>
        /// Unit container.
        /// </summary>
        private static IIoCWrapper wrapper;

        private readonly IUnityContainer container;
        /// <summary>
        /// Allows to use the lock for thread safty.
        /// </summary>
        private static readonly object padlock = new object();

        #endregion

        #region Constructors

        /// <summary>
        /// Prevents a default instance of the <see cref="IoCWrapper"/> class from being created.
        /// </summary>
        private IoCWrapper()
        {
            container = new UnityContainer();
        }

        #endregion

        #region Methods

        /// <summary>
        /// Creates or returns if already exists a new instance of IoC Container.
        /// Public reference to the unity container which will allow the ability to register instances or take other actions on the container.
        /// </summary>
        /// <returns>IoC container instance.</returns>
        public static IIoCWrapper Instance()
        {
            lock (padlock)
            {
                return wrapper ?? (wrapper = new IoCWrapper());
            }
        }

        /// <summary>
        /// Resolves the type parameter T to an instance of the appropriate type.
        /// </summary>
        /// <typeparam name="T">Type of object to return</typeparam>
        public T ResolveType<T>()
        {
            T ret = default(T);

            if (container.IsRegistered(typeof(T)))
            {
                ret = container.Resolve<T>();
            }

            return ret;
        }

        public T ResolveNamedType<T>(string name)
        {
            T ret = default(T);

            if (container.IsRegistered<T>(name))
            {
                ret = container.Resolve<T>(name);
            }

            return ret;
        }

        public void RegisterType<TInterface, TImplementation>()
            where TInterface : class where TImplementation : class, TInterface
        {
            container.RegisterType<TInterface, TImplementation>();
        }

        public void RegisterNamedType<TInterface, TImplementation>(string name)
            where TInterface : class where TImplementation : class, TInterface
        {
            container.RegisterType<TInterface, TImplementation>(name);
        }

        public void RegisterInstance<TInterface>(TInterface instance) 
        {
            container.RegisterInstance<TInterface>(instance);
        }

        #endregion
    }

The idea is to keep the client independent of underlying implementation of IoC so in future if we decide to swap Unity with Ninject or StructureMap or any other container we can just change the IoCWrapper.cs.

Update: 1 - Added the Usage..

In the application start method say in Global.asx.cs file or something.. we can call a static method say RegisterTypes()..

public void RegisterType()
        {
            IoCWrapper.Instance().RegisterType<ISomeInterface, SomeClass>();
            IoCWrapper.Instance().RegisterType<IDataAccess, CustomerDataAccess>();

            // for named types...
            IoCWrapper.Instance().RegisterType<IMortgageProductService, ShortTermProduct>("ShortTerm");
            IoCWrapper.Instance().RegisterType<IMortgageProductService, LongTermProduct>("LongTerm");
        }

Then in the client class we can have...

IDataAccess da = IoCWrapper.Instance().ResolveType<IDataAccess>();
IMortgageProductService da = IoCWrapper.Instance().ResolveType<IMortgageProductService>("ShortTerm");

Update 2: replaced

if (container.IsRegistered(typeof(T)))

with

if (container.IsRegistered<T>(name))

in public T ResolveNamedType<T>(string name) method.

\$\endgroup\$
  • 2
    \$\begingroup\$ I don't think this is a good idea. You are loosing a lot of unique features that make one framework better than the other. \$\endgroup\$ – t3chb0t Feb 19 '18 at 10:41
  • 2
    \$\begingroup\$ If your classes depend on this coantainer then you are not doing dependency injection but using a service locator. Could you add a usage example? Maybe I'm wrong. However usually you'd use a DI framework only in one place where you set it up and thus it's easy to exchange. Not all around the place. \$\endgroup\$ – t3chb0t Feb 19 '18 at 10:45
  • 1
    \$\begingroup\$ Ok, I was right. This is not DI, this is service locator... you're doing it wrong by polluting your code with the IoCWrapper. \$\endgroup\$ – t3chb0t Feb 19 '18 at 10:54
  • 1
    \$\begingroup\$ Your code should not know that you are using any DI framework. Currently it strongly depends on it. \$\endgroup\$ – t3chb0t Feb 19 '18 at 10:55
  • 1
    \$\begingroup\$ Let me ask you this question again: if you are implementing a single interface for any DI framework, why would you ever want to exchange it? No other framework can be better than the other one because they all are hidden behind a common abstraction and since you won't use a different framework (there is no reason to do so, it has not advantages) why would you need this wrapper? Usually you pick one of them because it can do something really cool, what you like most. But what do you like most about them now? What is your criteria for chosing THE one? \$\endgroup\$ – t3chb0t Feb 19 '18 at 11:08
4
\$\begingroup\$

The wrapper is very simple but I'd still change a few things to make it prettier, more reliable and more useful so these are my suggestions.


public static IIoCWrapper Instance()

I'd turn this method into a readonly property with a lazy backing field:

private static readonly Lazy<IIoCWrapper> wrapper = new Lazy<IIoCWrapper>(() => new IoCWrapper(), isThreadSafe: true);

public static IIoCWrapper Instance => wrapper.Value;

public T ResolveNamedType<T>(string name)
{
    T ret = default(T);

    if (container.IsRegistered(typeof(T)))
    {
        ret = container.Resolve<T>(name);
    }

    return ret;
}

I find this (and this and the other overload) should throw a strong exception containg both the name and the type when a type is not registered. This would make it easier to prevent the evil NullReferenceException.

public T ResolveNamedType<T>(string name)
{
    if (container.IsRegistered(typeof(T)))
    {
        return container.Resolve<T>(name);
    }

    throw new NamedTypeNotRegisteredException($"There is no type {typeof(T).Name} registered as {name}.");
}

public void RegisterType<TInterface, TImplementation>()
    where TInterface : class where TImplementation : class, TInterface
{
    container.RegisterType<TInterface, TImplementation>();
}

I'm not sure whether TInterface is really ment to be an interface but if so, then you should probably ensure that this is true typeof(TInterface).IsInterface. It would save a lot of headaches later.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for reviewing the code. I like to implement the changes that you suggested apart from change the Instance() method into a getter property and yes "TInterface is really ment to be an interface". \$\endgroup\$ – Yawar Murtaza Feb 19 '18 at 12:02
4
\$\begingroup\$

I agree with t3chb0t's comments, this sounds like a bad idea. You assume too much about what functionality you are going to need and what other containers are like.

1) Some containers require you to release components.

2) Some can (should) be disposed.

3) Some can (should) be configured via configuration files.

4) Some containers can be nested.

5) Some containers allow new registrations after first resolution, some do not.

6) Some, like say Castle.Windsor, have a concept of "installers", where you do not call RegisterType method directly, but use a custom IWindsorInstaller implementation, which does the registration.

And this list goes on and on. There are different component lifestyle models, different dependency resolution policies, different extension points, different... everything. Yes you have successfully abstracted out 4 most basic methods that pretty much every container has. But what next? You might think otherwise now, but sooner or later, you will need to use a custom factory for this component or specify a dependency for that component. And what will you do then? Will you create your own custom registration API, that you will then map to the API of underlying container? And then rewrite the entire mapping, when you do change the container? Well, that sounds bizarre to me.

TLDR: Just use containers the way they are supposed to be used. Not as a static service locator, but as a composition engine: isolate your registration logic and call Resolve method once to resolve a composition root. If you would ever want to change the container - yes you would have to rewrite your registration. Will it take some time? Yes, due to how different containers are. Is it harder than rewriting a wrapper, once registration logic become a bit more complex? God, no.

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