Kobe – Data Access done wrong
From the Kobe documentation:
The Repository pattern is applied to mediate access to the data store through a Repository provider.
From PoEAA:
A system with a complex domain model often benefits from a layer, such as the one provided by Data Mapper (165), that isolates domain objects from details of the database access code. In such systems it can be worthwhile to build another layer of abstraction over the mapping layer where query construction code is concentrated. This becomes more important when there are a large number of domain classes or heavy querying. In these cases particularly, adding this layer helps minimize duplicate query logic.
I think that Repository is a fad, which is why I don’t use it much anymore myself, and looking at the usage semantics of the “repositories” in Kobe, they certainly don’t fit the bill.
Repositories are here to encapsulate query logic, that is not the case with Kobe. Here is a partial view of some of the repositories that it have:
In general, looking at the any of the repositories in detail, I really want to cry. I mean, just take a look:
More to the point, let us look at GroupRepository.AcceptGroupInvite (I removed all the copy & paste crap to concentrate on the actual code):
GroupInvite groupInvite = _context.GroupInviteSet .Where(gi => gi.GroupInviteId == groupInviteId) .FirstOrDefault(); if (groupInvite == null) throw new DataException("Invalid Invite Id", null); groupInvite.InvitationStatuses = _context.InvitationStatusSet .Where(s => s.InvitationStatusName == "Accepted") .FirstOrDefault(); ; _context.SaveChanges(true);
There are so many things wrong with this approach that I don’t even know where to start.
You can see how the architects or developers resented having to use an ORM. If they couldn’t have stored procedures in the database, then by Jove and his keyboard, they will write stored procedures in code.
Kobe uses Entity Framework for data access, and they treat it as if it was a dataset. As an OR/M aficionado, I am insolated. This shows complete lack of understanding how Entity Framework work, doing a lot more work than you should, brute force & big hammer approach.
From an architectural perspective, most of what Kobe is doing is calling down to the repository to perform the data access details. Business logic, such as there is, is spread between the data access, the controllers and the service layer, in such a way that make is very hard to have a good idea about what is actually happening in the application.
Data access details are quite pervasive throughout the application, and the whole feeling of the application is of a circa 2001 sample app using stored procedures and hand rolled data access layers.
Comments
Would you mind writing some more about this in a new post? I'm intrigued.
How dare you say bad things about next sample from Microsoft? You barbarian, biting a hand that feeds you! MS programmers were working on it since 2001 so how can you expect the app to be modern and cutting edge?
What alternative would you use to a Repository? Just ORM code such as the NHibernate DetachedCriteria API, HQL or LINQ in your services? Or even in your controllers? I'm asking because I respect your opinion and I've been using repositories for a while now, and find that they're a pretty useful abstraction.
"Data access details are quite pervasive throughout the application, and the whole feeling of the application is of a circa 2001 sample app using stored procedures and hand rolled data access layers."
HAHA this is classic! Writing those factories is like chasing watching a cat chase its tail... go find a biz problem that needs data access, abstract it, go find a biz problem that needs data access, abstract it. Makes my hand shake just thinking about it =(
I agree with AdamB. Ayende, could you please expand on this?
Another agreement with AdamB :) I wanna know!
+1 on repo alternative.
Count me in with the RepositoryAsFad crowd...
+1 on repository alternative
Ditto on what everyone else is saying... :)
Popular demand!!! repository alternative!!!
I think the point that Ayende is making is that a lot (most) of those methods are really domain behavior that don't have anything to do with data access per se and thus shouldn't be part of the data access layer.
Without looking at the code I suspect that this codebase has an anemic domain model.
Who ever wrote that IUserRepository should be fired immediately. As a matter of fact, they shouldn't be allowed to touch code any more.
... Since everybody now an interface should have 100 methods...!
hmm.. interesting comments. I'm partial to Rails's ActiveRecord with dynamic accesors, but you don't get that in c#. Otherwise, I have no strong bias for or against repository, and am curious why so many comments are.
I also agree...I post on repository alternatives would be great. What's the strategy to keep the data access/orm choice (NHibernate / EF / Linq2Sql / etc) isolated and out of the "UI" layer?
Agree with the rest of the commenters - I'd love to see more explanation here. The rest of your Kobe posts have said "This is what is wrong, and this is how it should have been done". This one just says "This is wrong".
For those of us who don't use repositories and have never used EF, how should that that method be written? Or how should the repository be structured so the method was unnecessary?
Jon,
The problem is that this method should NOT be written.
This is not the proper layer for it.
As for constructing the repository for this. All you need is transparent persistence and you are golden.
Lot's of examples on the web of how not to do things...how about some examples of doing things right, or refactoring bad code like this into something that's ideal?
There seems plenty of people who are willing to moan at every app that is put out in the public domain and actually spend hours writing blog posts to pull them apart. What is funny is that they all state they cannot put together a best practise demo because nobody will pay them to do so. Surely the time spent moaning would be better spent providing?
re insolated = 'insolate - expose to the rays of the sun or affect by exposure to the sun' did you mean insulted?
Don't just say it's bad show how it can be improved, with examples. Good and bad examples should be shown when writing this type of article.
Comment preview