Ayende @ Rahien

It's a girl

Northwind Starter Kit Review: The parents have eaten sour grapes, and the children’s teeth are set on edge

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

In my previous posts, I focused mostly on the needlessness of the repositories implementation and why you want to avoid that (especially implementing it multiple times). In this post, I want to talk about other problems regarding the data access. In this case, the sudden urge to wash my hands that occurred when I saw this:

image

I mean, you are already using an OR/M. I don’t like the Repository implementation, but dropping down to SQL (and unparapetrized one at that) seems uncalled for.

By the way, the most logical reason for this to be done is to avoid mapping the Picture column to the category, since the OR/M in use here (EF) doesn’t support the notion of lazy properties.

Again, this is a problem when you are trying to use multiple OR/Ms, and that is neither required nor really useful.

Okay, enough about the low level data access details. On my next post I’ll deal with how those repositories are actually being used.

Comments

Stefan
01/12/2012 10:53 AM by
Stefan

Bestides this, I believe there's another thing that one should avoid at this level of abstraction: System.Drawing.Image as this has got to do with GDI stuff and this namespace doesn't make sense in the web world. Also, in WPF (unless you NEED to do some GDI stuff), you will not need System.Drawing at all and you might want to opt for something else just to display images on the screens... I haven't seen the place where this repository is used, but I guess it's only Winforms so perhaps a better name for this would be WinformsCustomerRepository? :)

NexusSharp
01/12/2012 12:54 PM by
NexusSharp

"By the way, the most logical reason for this to be done is to avoid mapping the Picture column to the category, since the OR/M in use here (EF) doesn’t support the notion of lazy properties."

Even that would be no reason to do it like that, why not use a binarydocument class that would contain the picture, this would allow lazy loading on the many-to-one side from category.

Bogdan Marian
01/12/2012 01:17 PM by
Bogdan Marian

@Ayende: Is "unparapetrized" a shortcut for unparameterized? ;)

Bogdan Marian
01/12/2012 01:17 PM by
Bogdan Marian

@Ayende: Is "unparapetrized" a shortcut for unparameterized? ;)

juakali
01/12/2012 02:18 PM by
juakali

I would only return the byte array. But hey how loosy do you want to go. I would say it's a nice readable and maintainable code and super fast. Or maybe cache (in memory) the categories and pick it from memory instead of DB.

Ignacio Fuentes
01/12/2012 02:20 PM by
Ignacio Fuentes

@ayende,

these posts are in now way useful!!

you are not saying how to do it right, just pointing out how wrong this approach is.

how is this helpful?

Nikolai Mynkow
01/12/2012 03:44 PM by
Nikolai Mynkow

@Ignacio

If you have to choose between 3 types of bread and I tell you that mouldy bread is not tasty was that helpful for you?

Ignacio Fuentes
01/12/2012 03:47 PM by
Ignacio Fuentes

@ Nikolai

thats not the case here at all...

ayende here is just telling me that mouldy bread is not good... hes not even mentioning other kinds of bread.

Nikolai Mynkow
01/12/2012 06:18 PM by
Nikolai Mynkow

Well, actually he is. good bread = use the orm directly, because it is actually a repository. If we are talking about NHibernate use ISession/ISession instead of leaky IRepository abstraction. Regards

João Bragança
01/12/2012 07:12 PM by
João Bragança

@Ignacio

There's some pretty good bread here: https://github.com/ayende/Alexandria

Karep
01/12/2012 08:34 PM by
Karep

Returning Image from repository seems so wrong.

Karep
01/12/2012 08:36 PM by
Karep

Ayende I don't know how lazy properties work in NHibernate but you can just create another object (Image) and add association from Category to Image and then you have lazy loading of image.

Karep
01/12/2012 08:38 PM by
Karep

If we have ActiveContext why not call ExecuteStoreCommand instead of creating command by hand? Also the reader is not disposed.

Ronnie Overby
01/12/2012 08:54 PM by
Ronnie Overby

Kids in the back, people. http://herdingcode.com/wp-content/uploads/HerdingCode-0131-Chris-Williams-and-Matthew-Podwysocki-on-the-Javascript-community.mp3

Bob
01/12/2012 10:38 PM by
Bob

@Ignacio

I hate to say it, but you must be new. This is what this guy does all the time. It's troll bait to get people mad so they talk about him. Of course he's an a**. Everyone already knows that.

Bek
01/12/2012 10:42 PM by
Bek

well technically, you can lazy load properties in EF. You have to split the table into two entities.

Phillip Haydon
01/13/2012 02:53 AM by
Phillip Haydon

@Bek - That would be changing your design to work around the limitations of EF rather than not having to worry about limitations to begin with.

Bob
01/13/2012 03:03 AM by
Bob

Yep use the context or session directly in your controller, lets just move back to code behinds while we are at it.

Steve Py
01/13/2012 05:11 AM by
Steve Py

@Phillip Well, it's 6 eggs one way, or half a dozen the other. I've seen plenty of problems with NHibernate implementations where the issues boil down to bad assumptions about what should be, or shouldn't be lazy loaded. Several were set up to either lazy load everything or nothing.

Having a design that explicitly designates an entity that's intended to be loaded separately may be a change in design, but not a bad change in design.

Phillip
01/13/2012 09:31 AM by
Phillip

@Steve - I'm not saying NHibernate is perfect or that NH is better than EF. All I mean is we shouldn't have to change our implementation just because something is not supported by a framework.

Rafal
01/13/2012 09:38 AM by
Rafal

I demand next article in this series. The plot is getting interesting and I'm sinking into it

Kijana Woodard
01/14/2012 02:22 AM by
Kijana Woodard

@Phillip And I think your comment is the linchpin to Ayende's entire argument. If it turns out that the ORM I've chosen has limitations and I've abstracted it away into a repository, I CAN'T work around it (i.e. ship code and make my client happy). Since I have this lovely abstractions, I say EF sucks and switch to NHibernate only to discover it has limitations in other areas. Ooops.

You really will need to be able to optimize queries for the situation at hand. The only code that knows that situation is the consuming code.

If you abstract away the ORM, you can't use it's features to optimize. If you expose it's features, you just leaked your ORM's implementation details through your abstractions and now you can't switch ORMs anymore.

So you might as well just use your ORM from the beginning and ship code.

EF or NHibernate both let us switch between many database engines. Once we work out all the issues of that abstraction for one ORM, what value do we get from switching our ORM?

Comments have been closed on this topic.