Ayende @ Rahien

It's a girl

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:

image

In general, looking at the any of the repositories in detail, I really want to cry. I mean, just take a look:

image

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

Nathan
04/17/2009 06:57 AM by
Nathan

I think that Repository is a fad, which is why I don’t use it much anymore myself

Would you mind writing some more about this in a new post? I'm intrigued.

Rafal
04/17/2009 07:25 AM by
Rafal

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?

AdamB
04/17/2009 08:09 AM by
AdamB

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.

CaliCoder
04/17/2009 08:17 AM by
CaliCoder

"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 =(

James
04/17/2009 08:25 AM by
James

I agree with AdamB. Ayende, could you please expand on this?

Nik
04/17/2009 08:40 AM by
Nik

Another agreement with AdamB :) I wanna know!

Nick
04/17/2009 09:07 AM by
Nick

+1 on repo alternative.

J Healy
04/17/2009 09:15 AM by
J Healy

Count me in with the RepositoryAsFad crowd...

Adriaan
04/17/2009 12:44 PM by
Adriaan

+1 on repository alternative

James Brechtel
04/17/2009 01:08 PM by
James Brechtel

Ditto on what everyone else is saying... :)

Flaker
04/17/2009 01:26 PM by
Flaker

Popular demand!!! repository alternative!!!

LukeB
04/17/2009 03:02 PM by
LukeB

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.

tawani
04/17/2009 03:04 PM by
tawani

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...!

josh
04/17/2009 03:19 PM by
josh

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.

Rob Gibbens
04/17/2009 03:20 PM by
Rob Gibbens

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?

Jon Galloway
04/17/2009 03:43 PM by
Jon Galloway

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?

Ayende Rahien
04/17/2009 03:48 PM by
Ayende Rahien

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.

Jim
04/17/2009 09:41 PM by
Jim

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?

redsquare
04/17/2009 11:12 PM by
redsquare

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?

stuartd
04/19/2009 12:06 AM by
stuartd

re insolated = 'insolate - expose to the rays of the sun or affect by exposure to the sun' did you mean insulted?

Dave Meah
04/19/2009 05:56 PM by
Dave Meah

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.

Comments have been closed on this topic.