Ayende @ Rahien

Hi!
My name is Ayende Rahien
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

@

Posts: 5,947 | Comments: 44,541

filter by tags archive

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

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

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

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

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

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

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

Bogan,

No, I meant to have IRepository -> RepositoryImpl

Why do you think this is a bad approach?

Ayende Rahien

Grimace,

Yes, == false is my own personal idiosyncrasy

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

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

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

A serif font would improve it.

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

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

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

"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

@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

@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

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

Guys,

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

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

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

You could make that way nastier by using LINQ to Objects

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

No future posts left, oh my!

RECENT SERIES

  1. RavenDB Sharding (3):
    22 May 2015 - Adding a new shard to an existing cluster, splitting the shard
  2. The RavenDB Comic Strip (2):
    20 May 2015 - Part II – a team in trouble!
  3. Challenge (45):
    28 Apr 2015 - What is the meaning of this change?
  4. Interview question (2):
    30 Mar 2015 - fix the index
  5. Excerpts from the RavenDB Performance team report (20):
    20 Feb 2015 - Optimizing Compare – The circle of life (a post-mortem)
View all series

RECENT COMMENTS

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats