Update: Andrea, the project’s leader has posted a reply to this series of posts.
I like to start reviewing applications from their database interactions. That it usually low level enough to tell me what is actually going on, and it is critical to the app, so a lot of thought usually goes there.
In good applications, I have hard time finding the data access code, because it isn’t there. It is in the OR/M or the server client API (in the case of RavenDB). In some applications, if they work against legacy databases or without the benefit of OR/M or against a strange data source (such as a remote web service target) may need an explicit data layer, but most don’t.
NSK actually have 5 projects dedicated solely to data access. I find this.. scary.
Okay, let me start outlying things in simple terms. You don’t want to do things with regards to data access the way NSK does them.
Let us explore all the ways it is broken. First, in terms of actual ROI. There is absolutely no reason to have multiple implementations with different OR/Ms. There is really not a shred of reason to do so. The OR/M is already doing the work of handling the abstraction from the database layer, and the only thing that you get is an inconsistent API, inability to actually important features and a whole lot more work that doesn’t' get you anything.
Second, there are the abstractions used:
I don’t like repositories, because they abstract important details about the way you work with the database. But let us give this the benefit of the doubt and look at the implementation. There is only one implementation of IRepository, which uses NHibernate.
As you can see, this is pretty basic stuff. You can also see that there are several methods that aren’t implemented. That is because they make no sense to a data. The reason they are there is because IRepository<T> inherits from ICollection<T>. And the reason for that is likely because of this:
Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects.
The fact that this is totally the wrong abstraction to use doesn’t enter to the design, it seems.
Note that we also violate the contract of ICollection<T>.Remove:
There are other reasons to dislike this sort of thing, but I’ll touch on that on my next post.
More posts in "Northwind Starter Kit Review" series:
- (26 Jan 2012) Conclusion
- (24 Jan 2012) That CQRS thing
- (23 Jan 2012) It is all about the services
- (20 Jan 2012) From start to finishing–tracing a request
- (18 Jan 2012) If you won’t respect the database, there will be pain
- (16 Jan 2012) Refactoring to an actual read model
- (13 Jan 2012) Data Access review thoughts
- (12 Jan 2012) The parents have eaten sour grapes, and the children’s teeth are set on edge
- (11 Jan 2012) Data Access and the essence of needless work, Part II
- (10 Jan 2012) Data Access and the essence of needless work, Part I