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.