Ayende @ Rahien

Refunds available at head office

Northwind Starter Kit Review: From start to finishing–tracing a request

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

One of the things that I repeatedly call out is the forwarding type of architecture, a simple operation that is hidden away by a large number of abstractions that serves no real purpose.

Instead of a controller, let us look at a web service, just to make things slightly different. We have the following:

image

Okay, let us dig deeper:

image

I really like the fact that this repository actually have have FindById method, which this service promptly ignores in favor of using the IQueryable<Customer> implementation. If you want to know how that is implemented, just look (using the EF Code First repository implementations, the others are fairly similar):

image

 

All in all, the entire thing only serves to make things harder to understand and maintain.

Does anyone really think that this abstraction adds anything? What is the point?!

Comments

Rafal
01/20/2012 10:04 AM by
Rafal

The point is to increase the sales of your ORM profiler.

dotnetchris
01/20/2012 02:33 PM by
dotnetchris

IMO, the inherent failure here is the dependency this author has taken: ICustomerRepository. ICustomerRepository then implements a generic repository through inheritance which is epic fail. Generic repositories only provide value if they're produced through composition, not inheritance.

If this code showed:

public CustomerDetailWindowWorkerServices(IRepository customerRepository)

it would likely have far more value.

Completing repository implementations through inheritance is a gigantic anemic model problem. Leaving you with tons of useless classes like:

public CustomerRepository : GenericRepository { Public Customer FindById(string id) { return base.FindById(id) } }

Or other noise abstractions. Or even entirely empty classes

public CustomerRepository : GenericRepository { }

^ can't get any more anemic than that.

dotnetchris
01/20/2012 02:48 PM by
dotnetchris

IMO, the inherent failure here is the dependency this author has taken: ICustomerRepository. ICustomerRepository then implements a generic repository through inheritance which is epic fail. Generic repositories only provide value if they're produced through composition, not inheritance.

If this code showed:

public CustomerDetailWindowWorkerServices(IRepository<Customer> customerRepository)

it would likely have far more value.

Completing repository implementations through inheritance is a gigantic anemic model problem. Leaving you with tons of useless classes like:

public CustomerRepository : GenericRepository<Customer> { Public Customer FindById(string id) { return base.FindById(id) } }

Or other noise abstractions. Or even entirely empty classes

public CustomerRepository : GenericRepository<Customer> { }

^ can't get any more anemic than that.

First post had all my generics butchered, hopefully this one works.

Dan
01/20/2012 03:35 PM by
Dan

@dotnetchris

I like your assertion that "Generic repositories only provide value if they're produced through composition, not inheritance." I've seen this done many times before and never been comfortable with it.

Would you propose that, instead of

CustomerRepository : GenericRepository{ }

Use something like:

CustomerRepository : ICustomerRepository { GenericRepository r {get;set;}

public Customer FindById(string id){ return r.FindById(id};

}

Or am I missing your point?

Lord spa
01/21/2012 08:38 AM by
Lord spa

Despite many worthy views to the contrary I stick to lines of code at my favourite metric. It encourages the team to stop writing rubbish that others have to decipher and maintain, and to cut out layers of abstraction when they find them.

Stan
01/22/2012 03:16 PM by
Stan

When your model is just a bag of properties and your application is just a CRUD you realy dont need repository. When you have complex model it is another story. It is a common case for commands when I should access one aggregate from other and than I prefer to inject repository into my entities instead of ISession. Just dont like to see NH use in a model. For queries I dont fetch model entities and than convert them to DTOs. I map DTOs to DB and directly use ISession in my query handlers.

Dmitry
01/23/2012 04:46 AM by
Dmitry

@Stan

You are not supposed to have data access code (whether it is repositories or ADO .NET classes) inside entities. That is the whole point of the POCO concept.

Dan Turner
01/23/2012 05:57 AM by
Dan Turner

So I'm guessing your problem is with the Repository and not the Service?

Because in my opinion that service performs a clear purpose, it constructs a view Customer. Admittedly it seems as though the service and view could have been named better. I am also more than happy to see code like this NOT happen in the body of a controller action for all but the most simplest of scenarios.

Dan Turner
01/23/2012 05:58 AM by
Dan Turner

constructs a view of a Customer*

Phil
01/25/2012 11:22 PM by
Phil

+1 to Dmitry. Having data access in your domain model is an absolute nightmare. Even though you can mock your DA interfaces it makes for way uglier tests.

Comments have been closed on this topic.