Ayende @ Rahien

It's a girl

Elegant Code

I just finished writing this, and I find it very pleasing to look at:

public class EntitiesToRepositories
{
	public static void Register(
		IWindsorContainer windsorContainer,
		ISessionFactory sessionFactory,
		Type repository,
		params Assembly[] assemblies
		)
	{
		if(typeof(IRepository<>).IsAssignableFrom(repository))
			throw new ArgumentException("Repository must be a type inheriting from IRepository<T>, " + 
"and must be an open generic type. Sample: typeof(NHRepository<>).
"); foreach (IClassMetadata meta in sessionFactory.GetAllClassMetadata().Values) { Type mappedClass = meta.GetMappedClass(EntityMode.Poco); if (mappedClass == null) continue; foreach (Type interfaceType in mappedClass.GetInterfaces()) { if(IsDefinedInAssemblies(interfaceType, assemblies)==false) continue; windsorContainer.Register( Component.For(typeof(IRepository<>).MakeGenericType(interfaceType)) .ImplementedBy(repository.MakeGenericType(interfaceType)) .CustomDependencies(Property.ForKey("ConcreteType").Eq(mappedClass)) ); } } } private static bool IsDefinedInAssemblies(Type type, Assembly[] assemblies) { return Array.IndexOf(assemblies, type.Assembly) != -1; } }

Comments

Bogdan Pietroiu
03/27/2008 09:40 AM by
Bogdan Pietroiu

Hi Ayende,

When you coded

Component.For(typeof(IRepository<>).MakeGenericType(interfaceType)).ImplementedBy(repository.MakeGenericType(interfaceType))

you meant

Component.For(typeof(IRepository<>).MakeGenericType(interfaceType)).ImplementedBy(repository.MakeGenericType(mappedClass))

imho, I am afraid that this approach does not play well with separation of concerns alowing a mix of data model layer and business layer

json
03/27/2008 11:27 AM by
json

First thoughts when seeing your codesnippet, either:

a) The indent-formatting is messed up in the post (way to deep)

..or...

b) You work at a wide, wide, widescreen ;-)

Personally I prefer 4 chars deep indents

Grimace of Despair
03/27/2008 11:38 AM by
Grimace of Despair

Maybe a stupid question, but do you consciously use "== false" instead of negation for readability? Of is it just what popped to mind?

Francois Tanguay
03/27/2008 12:02 PM by
Francois Tanguay

And I imagine your first if was negated. Otherwsie you'll throw the exception quite often.

Fabian Schmied
03/27/2008 12:20 PM by
Fabian Schmied

Have you actually managed to get that first block of code to throw? Francois seems to be right - to fit the exception message, the if condition should be negated.

But even so, typeof(IRepository<>).IsAssignableFrom (repository) only yields true if repository == typeof (IRepository<>). Which is probably not what you meant.

Also, in general, do you think that continues are easier to read than if blocks that span more than one line? (This is a serious question.)

Francois Tanguay
03/27/2008 12:50 PM by
Francois Tanguay

Good catch. Ah the joy of of open generic types and interfaces. It is anything but predictable.

This little code snippet show well how much hidden complexity there is in any amount of code.

What seems elegant at first, end up being anything but it.

On a side note, there is plenty of room for some LINQification.

Ayende Rahien
03/27/2008 01:13 PM by
Ayende Rahien

Bogan,

No, I meant to have IRepository -> RepositoryImpl

Why do you think this is a bad approach?

Ayende Rahien
03/27/2008 01:27 PM by
Ayende Rahien

Grimace,

Yes, == false is my own personal idiosyncrasy

Ayende Rahien
03/27/2008 01:40 PM by
Ayende Rahien

Francois,

Yes, it should be negated, but the exact semantics of how to do that escape me. Open generics types are a wholly mess.

Ayende Rahien
03/27/2008 01:41 PM by
Ayende Rahien

Yes, it is really annoying:

Type repository = typeof(NHRepository<>);

typeof(IRepository<>).IsAssignableFrom (repository) - false

repository.IsAssignableFrom (typeof(IRepository<>)) - false

mixing a lot of GetGenericTypeDefinition - false

Ayende Rahien
03/27/2008 01:42 PM by
Ayende Rahien

Fabian,

Yes, good catch. A case of two bugs masking each other :-)

The real code it is only a single line, I broke it up for presentation

herr ziffer
03/27/2008 01:45 PM by
herr ziffer

A serif font would improve it.

Francois Tanguay
03/27/2008 01:50 PM by
Francois Tanguay

You need to go through all interfaces of the type, get the GenericTypeDefinition of the Generic Interfaces as once implemented, they are closed and then do equality comparison

Assert.Equal(

typeof(IRepository<>),

typeof<NHibernateRepository>.GetInterfaces()[0].GetGenericTypeDefinition());

Colin Jack
03/27/2008 02:07 PM by
Colin Jack

How would this approach handle the situations where multiple entities do implement the same interface.

Ayende Rahien
03/27/2008 02:12 PM by
Ayende Rahien

That is why you are handing it a list of domain assemblies.

It doesn't make sense for entities to implement the same domain interface. Domain interface are not generic in nature.

Colin Jack
03/27/2008 02:22 PM by
Colin Jack

"It doesn't make sense for entities to implement the same domain interface. Domain interface are not generic in nature."

Not saying they are generic but you might have IAsset which can be implemented by multiple mapped classes. Quite rare I guess though, and you could deal with those cases differently.

Francois Tanguay
03/27/2008 03:03 PM by
Francois Tanguay

@Coliln: Funny, I have exactly that. ITrade and IAsset are implemented by multiple domain classes...

public class Bond : IAsset {}

public class Derivative : ITrade {}

public class SwapLeg : IAsset {}

public class BondTrade : ITrade {}

Colin Jack
03/27/2008 03:11 PM by
Colin Jack

@Francois

Yeah, thinking after I posted it really isn't all that rare, for example we have multiple types of fee recipient so we have IFeeRecipient.

Bruno
03/27/2008 03:13 PM by
Bruno

I often use C(#) embedded statements, and believe it´s a great combination with C(#) powefull operators (of course, always trying to preserve readability).

If it was VB we were ended with 2 times the lines of code, may be.

I just don´t like to replace (!) with (== false), because its more verbose. But I believe CSC.exe handle as they were the same instruction, so it´s just a preference. Isn´t it?

Ayende Rahien
03/27/2008 03:18 PM by
Ayende Rahien

Guys,

You convinced me. I changed that to a predicate, so you can control the logic externally

Bruno
03/27/2008 03:31 PM by
Bruno

I know many people don´t like embedded statements, here is an example, it doesn´t have any curly braces in the method´s body but does a lot of things. I´ve wrote it, believe it´s elegant:

    public object Get(string baseKey, TimeSpan expiration, CacheItemGetter getCacheItem)

    {

        if (string.IsNullOrEmpty(baseKey))

            throw new ArgumentException("baseKey");

        if (getCacheItem == null)

            throw new ArgumentNullException("getCacheItem");


        object cacheItem = null;


        lock (BuildFullCacheItemKey(baseKey))

            if ((cacheItem = Get(baseKey)) == null && (cacheItem = getCacheItem()) != null)

                Add(baseKey, expiration, cacheItem);


        return cacheItem;

    }
Pat Gannon
03/27/2008 04:24 PM by
Pat Gannon

The cyclomatic complexity of this method is a little high for my tastes, due to the double-nested control structure (an 'if' in a 'foreach' in a 'foreach'). I would have broken Register up into two methods - probably a public Register and a private RegisterInterfaces that encapsulates the inner 'foreach' and the 'if'.

J.P. Hamilton
03/27/2008 05:03 PM by
J.P. Hamilton

You could make that way nastier by using LINQ to Objects

Comments have been closed on this topic.