While working with the IUnityContainer interface from Microsoft's Patterns and Practices team, I decided that it was well worth a post on how not to design interfaces. Recent discussion amongst Rhys and Colin (here as well) have been interesting but I imagine most would agree that both arguments are probably fighting over which polish to use on their code. If these are the big battles you have to face at work then I am jealous.
Introducing IUnityContainer ....
42 members are defined on this interface. Fail.
With much restraint I wont go on about it and flame the guys who write it. Most people know of my disdain for the P&P team at Microsoft. So what tips do I give to make this a usable again? Lets break down the solution into a few tips:
- Extension methods
- Cohesion
- Intention revealing interfaces
How do each of these help us. Let look at some code I was writing pre-extension methods. It was a validation interface that had 2 methods, Validate and Demand
public interface IValidator<T>
{
IEnumerable<ValidationErrors> Validate(T item);
///<exception cref="ValidationException">Thrown when the item is not valid</exception>
void Demand(T item);
}
The problem with this interface is that all implementations of the interface would/could implement Demand the same way; Make a call to Validate(T item) and if any ValidationErrors came back throw an exception with the validation failures. Time ticks by and I realise the now that I have extension methods in C# 3.0 (.NET 3.5). I only need to have the Validate method on my interface now and provide the Demand implementation as an extension method. The code now becomes:
public interface IValidation<T>
{
/// <summary>
/// Validates the specified item, returning a collection of validation errors.
/// </summary>
/// <param name="item">The item to validate.</param>
/// <returns>Returns a <see cref="T:ArtemisWest.ValidationErrorCollection"/> that is empty for a valid item,
/// or contains the validation errors if the item is not in a valid state.</returns>
/// <remarks>
/// Implementations of <see cref="T:ArtemisWest.IValidation`1"/> should never return null from the validate method.
/// If no validation errors occured then return an empty collection.
/// </remarks>
IEnumerable<ValidationErrors> Validate(T item);
}
public static class ValidatorExtensions
{
/// <summary>
/// Raises a <see cref="T:ArtemisWest.ValidationException"/> if the <paramref name="item"/> is not valid.
/// </summary>
/// <typeparam name="T">The type that the instance of <see cref="T:IValidation`1"/> targets.</typeparam>
/// <param name="validator">The validator.</param>
/// <param name="item">The item to be validated.</param>
/// <exception cref="T:ArtemisWest.ValidationException">
/// Thrown when validating the <paramref name="item"/> returns any errors. Only the first
/// validation error is raised with the exception. Any validation errors that are marked as
/// warnings are ignored.
/// </exception>
public static void Demand<T>(this IValidation<T> validator, T item)
{
foreach (var error in validator.Validate(item))
{
if (!error.IsWarning)
{
throw new ValidationException(error);
}
}
}
}
Then end result is that the API feels the same as I have access to both methods, but the cost of implementing my interface is reduced to just its core concern.
So, extension methods is one trick we have in the bag. Next cohesion.
The recent discussion between Rhys and Colin is "how many members belong on an interface?" I think both will agree that answer is not 42. Juval Lowy made a great presentation at TechEd2008 on Interfaces and that we should be aiming for 3-7 members per interface. This provides us with a level of cohesion and a low level of coupling. Lets look at some of the members on the IUnityContainer Interface.
- 8 overloads of RegisterInstance
- 16 overloads of RegisterType
- Add/Remove "Extensions" methods
- 4 overloads of BuildUp
- 2 overloads of ConfigureContainer
- CreateChildContainer
- 4 overloads of Resolve
- 2 overloads of ResolveAll
- A TearDown method
- and a Parent property
Whew! Well how can we tame this beast? When I look at this interface I see certain groups that look like they are related in usage. They would be the
- Register and Resolve functionality
- And and Remove extensions functionality
- Build up and teardown functionality
- Container hierarchy functionality (Parent and CreateChildContainer)
- Container configuration
Interestingly on our current project we only use the Register/Resolve functionality.
These five groups to me have some level of cohesion. That to me then makes them a candidate for there own interfaces. The big giveaway being that I use unity quite successfully but never use 4/5 of the functionality defined on this interface. So our 2nd tip would be to split out these groups on functionality into there own interfaces.
But what do we name the new interfaces? This is our 3rd tip:
Intention revealing interfaces.
Looking at my list I would imagine some useful names could be:
- IContainer
- IExtensionContainer
- ILifecycleContainer
- INestedContainer
- IConfigurableContainer
To be honest, I have put little thought into these names. Normally I would put a LOT of effort into getting the naming right, but I don't work on the P&P team so these changes will never be done so why waste my time?
Edit: My laziness here really does take the wind our of the sails of this argument. Sorry.
Ok so how can we bring this all together? My proposal would be to have 6 interfaces
- IContainer
- IExtensionContainer
- ILifecycleContainer
- INestedContainer
- IConfigurableContainer
- IUnityContainer : IContainer, IExtensionContainer, ILifecycleContainer, INestedContainer, IConfigurableContainer
Next I would create some extension methods to deal with the silly amount of duplication the multiple overloads incur. Looking at the implementation in UnityContainerBase I would think that all of these methods could be candidates for extension methods
public abstract class UnityContainerBase : IUnityContainer, IDisposable
{
//prior code removed for brevity
public IUnityContainer RegisterInstance<TInterface>(TInterface instance)
{
return this.RegisterInstance(typeof(TInterface), null, instance, new ContainerControlledLifetimeManager());
}
public IUnityContainer RegisterInstance<TInterface>(string name, TInterface instance)
{
return this.RegisterInstance(typeof(TInterface), name, instance, new ContainerControlledLifetimeManager());
}
public IUnityContainer RegisterInstance<TInterface>(TInterface instance, LifetimeManager lifetimeManager)
{
return this.RegisterInstance(typeof(TInterface), null, instance, lifetimeManager);
}
public IUnityContainer RegisterInstance(Type t, object instance)
{
return this.RegisterInstance(t, null, instance, new ContainerControlledLifetimeManager());
}
public IUnityContainer RegisterInstance<TInterface>(string name, TInterface instance, LifetimeManager lifetimeManager)
{
return this.RegisterInstance(typeof(TInterface), name, instance, lifetimeManager);
}
public IUnityContainer RegisterInstance(Type t, object instance, LifetimeManager lifetimeManager)
{
return this.RegisterInstance(t, null, instance, lifetimeManager);
}
public IUnityContainer RegisterInstance(Type t, string name, object instance)
{
return this.RegisterInstance(t, name, instance, new ContainerControlledLifetimeManager());
}
//Remaining code removed for brevity
}
all of these methods just delegate to the one method overload left as abstract
public abstract IUnityContainer RegisterInstance(
Type t,
string name,
object instance,
LifetimeManager lifetime);
The obvious thing to do here is to just make all of these extension methods in the same namespace as the interfaces
public static class IContainerExtensions
{
public IContainer RegisterInstance<TInterface>(this IContainer container, TInterface instance)
{
return container.RegisterInstance(typeof(TInterface), null, instance, new ContainerControlledLifetimeManager());
}
public IContainer RegisterInstance<TInterface>(this IContainer container, string name, TInterface instance)
{
return container.RegisterInstance(typeof(TInterface), name, instance, new ContainerControlledLifetimeManager());
}
public IContainer RegisterInstance<TInterface>(this IContainer container, TInterface instance, LifetimeManager lifetimeManager)
{
return container.RegisterInstance(typeof(TInterface), null, instance, lifetimeManager);
}
public IContainer RegisterInstance(this IContainer container, Type t, object instance)
{
return container.RegisterInstance(t, null, instance, new ContainerControlledLifetimeManager());
}
public IContainer RegisterInstance<TInterface>(this IContainer container, string name, TInterface instance, LifetimeManager lifetimeManager)
{
return container.RegisterInstance(typeof(TInterface), name, instance, lifetimeManager);
}
public IContainer RegisterInstance(this IContainer container, Type t, object instance, LifetimeManager lifetimeManager)
{
return container.RegisterInstance(t, null, instance, lifetimeManager);
}
public IContainer RegisterInstance(this IContainer container, Type t, string name, object instance)
{
return container.RegisterInstance(t, name, instance, new ContainerControlledLifetimeManager());
}
}
This now reduces our IContainer Interface to just the one method
public interface IContainer
{
IContainer RegisterInstance(Type t, string name, object instance, LifetimeManager lifetime);
}
One thing to note here is that we have broken the contract because we now return IContainer not IUnityContainer. We will come back to this problem later.
If we then follow suit on the other interfaces we have created, we will have 6 interfaces that look like this:
public interface IContainer
{
IContainer RegisterType(Type from, Type to, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers);
IContainer RegisterInstance(Type t, string name, object instance, LifetimeManager lifetime);
object Resolve(Type t, string name);
IEnumerable<object> ResolveAll(Type t);
}
public interface IExtensionContainer
{
IExtensionContainer AddExtension(UnityContainerExtension extension);
IExtensionContainer RemoveAllExtensions();
}
public interface ILifecycleContainer
{
object BuildUp(Type t, object existing, string name);
void Teardown(object o);
}
public interface INestedContainer
{
INestedContainer CreateChildContainer();
INestedContainer Parent { get; }
}
public interface IConfigurableContainer
{
object Configure(Type configurationInterface);
}
public interface IUnityContainer : IDisposable, IContainer, IExtensionContainer, ILifecycleContainer, INestedContainer, IConfigurableContainer
{}
So now we have some much more managable interfaces to work with. However, we have broken the feature that it had before of returning IUnityContainer from each method. You may ask why would you return the instance when clearly you already have the instance? By doing so you can create a fluent interface. See my other post on Fluent interfaces and DSLs for more information.
Now that we have removed all the noise from the interfaces we actually have a reasonable number of members to work with. This makes me think that maybe we can refactor back to a single interface? Lets have a look:
public interface IUnityContainer : IDisposable
{
IUnityContainer RegisterType(Type from, Type to, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers);
IUnityContainer RegisterInstance(Type t, string name, object instance, LifetimeManager lifetime);
object Resolve(Type t, string name);
IEnumerable<object> ResolveAll(Type t);
IUnityContainer AddExtension(UnityContainerExtension extension);
IUnityContainer RemoveAllExtensions();
object BuildUp(Type t, object existing, string name);
void Teardown(object o);
IUnityContainer Parent { get; }
IUnityContainer CreateChildContainer();
object Configure(Type configurationInterface);
}
Well that is 13 members, which is above my happy limit of 10 and nearly double my ideal limit of 7, however...I think this would be a vast improvement on the silly interface that currently exists with its 42 members.
Just for fun here are the Extension methods that would complete the interface to bring it back to feature complete.
public static class IUnityContainerExtentions
{
public static IUnityContainer AddNewExtension<TExtension>(this IUnityContainer container) where TExtension : UnityContainerExtension, new()
{
return container.AddExtension(Activator.CreateInstance<TExtension>());
}
public static T BuildUp<T>(this IUnityContainer container, T existing)
{
return (T)container.BuildUp(typeof(T), existing, null);
}
public static object BuildUp(this IUnityContainer container, Type t, object existing)
{
return container.BuildUp(t, existing, null);
}
public static T BuildUp<T>(this IUnityContainer container, T existing, string name)
{
return (T)container.BuildUp(typeof(T), existing, name);
}
public static TConfigurator Configure<TConfigurator>(this IUnityContainer container) where TConfigurator : IUnityContainerExtensionConfigurator
{
return (TConfigurator)container.Configure(typeof(TConfigurator));
}
public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, TInterface instance)
{
return container.RegisterInstance(typeof(TInterface), null, instance, new ContainerControlledLifetimeManager());
}
public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, string name, TInterface instance)
{
return container.RegisterInstance(typeof(TInterface), name, instance, new ContainerControlledLifetimeManager());
}
public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, TInterface instance, LifetimeManager lifetimeManager)
{
return container.RegisterInstance(typeof(TInterface), null, instance, lifetimeManager);
}
public static IUnityContainer RegisterInstance(this IUnityContainer container, Type t, object instance)
{
return container.RegisterInstance(t, null, instance, new ContainerControlledLifetimeManager());
}
public static IUnityContainer RegisterInstance<TInterface>(this IUnityContainer container, string name, TInterface instance, LifetimeManager lifetimeManager)
{
return container.RegisterInstance(typeof(TInterface), name, instance, lifetimeManager);
}
public static IUnityContainer RegisterInstance(this IUnityContainer container, Type t, object instance, LifetimeManager lifetimeManager)
{
return container.RegisterInstance(t, null, instance, lifetimeManager);
}
public static IUnityContainer RegisterInstance(this IUnityContainer container, Type t, string name, object instance)
{
return container.RegisterInstance(t, name, instance, new ContainerControlledLifetimeManager());
}
public static IUnityContainer RegisterType<T>(this IUnityContainer container, params InjectionMember[] injectionMembers)
{
return container.RegisterType(typeof(T), null, null, null, injectionMembers);
}
public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, params InjectionMember[] injectionMembers) where TTo : TFrom
{
return container.RegisterType(typeof(TFrom), typeof(TTo), null, null, injectionMembers);
}
public static IUnityContainer RegisterType<T>(this IUnityContainer container, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
{
return container.RegisterType(typeof(T), null, null, lifetimeManager, injectionMembers);
}
public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers) where TTo : TFrom
{
return container.RegisterType(typeof(TFrom), typeof(TTo), null, lifetimeManager, injectionMembers);
}
public static IUnityContainer RegisterType<T>(this IUnityContainer container, string name, params InjectionMember[] injectionMembers)
{
return container.RegisterType(typeof(T), null, name, null, injectionMembers);
}
public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, string name, params InjectionMember[] injectionMembers) where TTo : TFrom
{
return container.RegisterType(typeof(TFrom), typeof(TTo), name, null, injectionMembers);
}
public static IUnityContainer RegisterType(this IUnityContainer container, Type t, params InjectionMember[] injectionMembers)
{
return container.RegisterType(t, null, null, null, injectionMembers);
}
public static IUnityContainer RegisterType<T>(this IUnityContainer container, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
{
return container.RegisterType(typeof(T), null, name, lifetimeManager, injectionMembers);
}
public static IUnityContainer RegisterType<TFrom, TTo>(this IUnityContainer container, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers) where TTo : TFrom
{
return container.RegisterType(typeof(TFrom), typeof(TTo), name, lifetimeManager, injectionMembers);
}
public static IUnityContainer RegisterType(this IUnityContainer container, Type t, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
{
return container.RegisterType(t, null, null, lifetimeManager, injectionMembers);
}
public static IUnityContainer RegisterType(this IUnityContainer container, Type t, string name, params InjectionMember[] injectionMembers)
{
return container.RegisterType(t, null, name, null, injectionMembers);
}
public static IUnityContainer RegisterType(this IUnityContainer container, Type from, Type to, params InjectionMember[] injectionMembers)
{
container.RegisterType(from, to, null, null, injectionMembers);
return container;
}
public static IUnityContainer RegisterType(this IUnityContainer container, Type t, string name, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
{
return container.RegisterType(t, null, name, lifetimeManager, injectionMembers);
}
public static IUnityContainer RegisterType(this IUnityContainer container, Type from, Type to, LifetimeManager lifetimeManager, params InjectionMember[] injectionMembers)
{
return container.RegisterType(from, to, null, lifetimeManager, injectionMembers);
}
public static IUnityContainer RegisterType(this IUnityContainer container, Type from, Type to, string name, params InjectionMember[] injectionMembers)
{
return container.RegisterType(from, to, name, null, injectionMembers);
}
public static T Resolve<T>(this IUnityContainer container)
{
return (T)container.Resolve(typeof(T));
}
public static T Resolve<T>(this IUnityContainer container, string name)
{
return (T)container.Resolve(typeof(T), name);
}
public static object Resolve(this IUnityContainer container, Type t)
{
return container.Resolve(t, null);
}
public static IEnumerable<T> ResolveAll<T>(this IUnityContainer container)
{
//return new <ResolveAll>d__0<T>(-2) { <>4__this = this };
//This implementation requires more effort than I am willing to give (6hours till my holiday!)
}
}
Bloody hell. Imagine trying to implement that on every class that implemented the interface!
While I know this entire post is academic as we cant change Unity, but I hope that sews some seeds for other developers so that when they design their next interface it wont have silly overloads that just make the consuming developer's job harder than it ought to be.
