Ayende @ Rahien

Refunds available at head office

Northwind Starter Kit Review: Data Access review thoughts

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

In my last few posts, I have gone over the data access strategy used in NSK. I haven’t been impressed. In fact, I am somewhere between horrified, shocked and amused in the same way you feel when you see a clown slipping on a banana peel.  Why do I say that? Let us trace a single call from the front end all the way to the back.

The case in point, CatalogController.Product(id) action. This is something that should just display the product on the screen, so it should be fairly simple, right? Here is how it works when drawn as UML:

image

To simplify things, I decided to skip any method calls on the same objects (there are more than a few).

Let me show you how this looks like in actual code:

image

Digging deeper, we get:

image

We will deal with the first method call to CatalogServices now, which looks like:

image

I’ll skip going deeper, because this is just a layer of needless abstraction on top of Entity Framework and that is quite enough already.

Now let us deal with the second call to CatalogServices, which is actually more interesting:

image

Note the marked line? This is generating a query. This is interesting, because we have already loaded the product. There is no way of optimizing that, of course, because the architecture doesn’t let you.

Now, you need all of this just to show a single product on the screen. I mean, seriously.

You might have noticed some references to things like Read Model in the code. Which I find highly ironic. Read Models are about making the read side of things simple, not drowning the code in abstraction on top of abstraction on top of abstraction.

In my next post, I’ll show a better way to handle this scenario. A way that is actually simpler and make use an of actual read model and not infinite levels of indirection.

Comments

Frans Bouma
01/13/2012 10:48 AM by
Frans Bouma

Maybe it's me but I fail to see the point of this 'review'. All I see is you bashing someone's code which was written for the most time a long time ago and which isn't really production code. Sure it's used to illustrate a point which isn't really that great, but that's actually subjective.

I don't like the over-the-top over-engineered architecture, but one can describe that in 1 post. You go on-and-on about details which gets me the feeling you want to bash the author personally: the 'best' thing one can learn from these posts is how not to build your software, but I think the author himself already knows that.

The posts furthermore suggests you think you have the authority to judge other people's code. While everyone's entitled to their opinion about anything, and therefore you too, isn't it a little 'holier than thou' when you dig so deep into someone else's code just to get more visitors on your blog? After all, one only has to take a little peek into the NHibernate code base to know your code isn't always up to par either.

Just stay a gentleman, Oren. There are no 2 developers on this planet who like eachother's code: there's always something wrong, they always would have done things differently.

tobi
01/13/2012 10:50 AM by
tobi

This is actually the style of code I am looking at right now.

In my case, all classes including "Services", "Managers", "Repositories" and DTOs are ... interfaces. With one implementation (or a second which has only NotImplemented stub method).

Pete McEvoy
01/13/2012 11:20 AM by
Pete McEvoy

Do you think you could show how you would do this using Raven? I'm all for simplicity and think that we need to start seeing concrete examples of how to implement using RavenDB which would perhaps give the doubters pause for thought?

Matt
01/13/2012 11:28 AM by
Matt

What I would like to see from this review is how it SHOULD be done.

For example, you dismiss the repository pattern, but what are the alternatives? For example, in an ASP.NET web application you have controllers. I do NOT want to see this code in my controllers:

var sessionFactory = CreateSessionFactory();

using (var session = sessionFactory.OpenSession()) { using (var transaction = session.BeginTransaction()) { // do a large amount of work

// save entities
session.SaveOrUpdate(myEntity);

transaction.Commit();

} }

That is ugly, repetetive code. I want in my service methods to Get, update, save, and not have to worry about the above.

Further, if my aggregate root query should exclude entities that have, for example, and IsActive = false flag, I also don't want to repeatedly exclude the IsActive = false entities. Using the repository pattern I can expose my Get method where internally it ALWAYS does this.

Although his solution is messy, I get the feeling this is what the author was trying to do. How would you recommend do this?

Let's see some constructive advice please.

linkgoron
01/13/2012 11:55 AM by
linkgoron

@pete this isn't about RavenDB, it's about misusing Entity framework. Using RavenDB the same way would be just as bad.

@Matt (I'm presuming we're talking about ASP.NET MVC3)

why not use IOC to inject your Session directly into the Controller class? why not have a filter that automatically opens a transaction every time the controller has a Session injected into it?

Also, EF/Nhibernate sessiosn by themselves implement the repository pattern, just use them directly. You can always add extension methods (You can look at the Racoon Blog repo, I believe they also have a few queries that they've added as extension methods).

and, IMO, you can always use a thin layer of abstraction, that doesn't abstract the fact that you're using EF/Nhibernate/Raven/Whatever, but helps you fine tune the API to your needs.

Bryan
01/13/2012 11:59 AM by
Bryan

Frans, you said it beautifully. His point is reasonable, but his approach is awful and filled with hubris and after so many of these reviews it's borderline beating a dead horse. I am this close to unsubscribing if it wasn't for all the other useful stuff he actually does post once in awhile.

afif
01/13/2012 01:48 PM by
afif

Oren, what do you use for your sequence diagrams?

Ayende Rahien
01/13/2012 01:50 PM by
Ayende Rahien

Afif, That seq diagram was generated by VS architecture tools

Matt
01/13/2012 02:19 PM by
Matt

@linkgoron

Yes, IoC does help with ISession creation, but I still like a repository pattern to separate commands from queries.

This is hard to explain succinctly, but say you have a windows service and a web project (yes, MVC3). Both use shared services that handle your domain commands. As the creator of the API, you do not know what COMBINATIONS of commands are required by the various platforms, you just provide the domain layer.

If, for example, there are two service methods that can be used separately in some instances, or together in others. You don't want a transaction opened in each case, but you HAVE to, because they can be called independently and NHib doesn't supported nested transactions.

I would like to see good advice on how these layers SHOULD be implemented, taking into account the following:

  • how to implement a service layer, where the exposed methods can be used independently or together (when is the right time to create transactions, sessions, load entities, etc)
  • where the BLL can be used from different tiers e.g. you want to put ISession objects in a "using" statement for a desktop app, but you don't want to do that in a web app - you want it cleaned up at the end of an Http web request using StructureMap or whatever.

I'm waffling now, but all I'm saying is I'd like to see more suggestions on best practice - what has been done in the Starter Kit project and how he SHOULD have done it, instead of just nailing the poor guy.

Derek
01/13/2012 02:22 PM by
Derek

http://community.devexpress.com/blogs/seth/archive/2011/03/09/interview-with-ayende-rahien-aka-oren-eini.aspx

Interesting take on the repo pattern.

Felice Pollano
01/13/2012 02:23 PM by
Felice Pollano

@Frans you are correct, but the NSK project is declared to be: "intended to be used as a blueprint when designing and implementing a .NET layered application architecture.", it is not just a someone else messy code...

Joe
01/13/2012 03:53 PM by
Joe

"In my next post, I’ll show a better way to handle this scenario. A way that is actually simpler and make use an of actual read model and not infinite levels of indirection."

Thanks.

Frans Bouma
01/13/2012 04:10 PM by
Frans Bouma

@Felice Declared by who? The author? So what! There are a lot of repositories on e.g. github where the author/developer thinks it's gods gift to humanity while it's a stinking pile of goo. Bashing that doesn't make the world better. It only makes the basher look like a person who thinks he never wrote bad code nor claimed the same thing.

Sure it's extremely over-engineered and therefore isn't the best choice as an example, but you can tell that 1 sentence (I just did) and move on.

Abdu
01/13/2012 06:00 PM by
Abdu

@Frans Like anything in life, pick the good and leave the bad behind. Putting his style aside, Ayende is one of the most .NET prolific bloggers and I thank him for his posts.

Note1: I wrote a comment very similar to yours (http://ayende.com/blog/152706/application-review-northwind-starter-kit).

Note2: BTW, I am interested in see a comparison between your profiler an Ayende's. (I don't mean one by you)

Larz
01/13/2012 06:23 PM by
Larz

This series is awful, and smacks of cyber-bullying. i'm gonna buy a copy of Raven so I know I'm safe and he won't attack my code.

Janus Knudsen
01/13/2012 07:05 PM by
Janus Knudsen

Frans Bouma, Bryan... I fully respect your point of views, but this blog isn't about pedagogical instructions and avoid calling bad code for bad code.

I think it's a blog for serious people who are willing to discuss and learn from each other, if some one wants a more educational soft approach they should attend a seminar instead.

Unfortunately the world is overflowed with people calling them self for developers, where in fact they only produce bad and smelly code. These people are producing bad code and gives software projects a bad reputation. Why do you think nearly all public software projects takes twice as long as supposed and drain the public cent by cent?

Tonio
01/13/2012 07:16 PM by
Tonio

+1 for Abdu

Nick
01/13/2012 08:00 PM by
Nick

Regardless of context, this is a great post isn't it? Showing us precisely how we can abstract data access so far down the layers we don't realise what calls are being made to the database by our high-level code.

When we suddenly do realise, we've got to twist all those layers to gain performance...or in many case die trying.

I've written a lot of code with these abstractions, and it has been Ayende's posts that have taught me to really consider adding all those layers.

Btw, Ayende has been on this developer's life and he stressed these weren't personal attacks.

I shall remain impartial and continue to learn from "the master".

comment
01/13/2012 10:09 PM by
comment

horrible architecture. i'm sorry for the poor coders that need to maintain this layers of shit

flukus
01/13/2012 10:54 PM by
flukus

A lot of us have to work with architecture this bad. Showing bad examples is just as important as showing good examples, it can help you realize when your going/gone down the wrong path.

gunteman
01/14/2012 01:33 AM by
gunteman

@Frans and Bryan

I wholeheartedly agree, but to fling some cold truth in the other direction, asking Ayende to be a gentleman is pointless. Such skills are not his forte. Zing.

Most of the bashing and bullying that happens on the net (and elsewhere) comes from people who can't assert themselves in any other way than by belittling others. Ayende is most certainly not in that position. Most of us envy his skills and insight. Still, for some reason, this continues to happen.

Also, I actually think this is hurting Hibernating Rhinos' bottom line. I know I'm not the only one who's very reluctant to send any money the way of such behavior. My loss, I guess, but still...

Felice Pollano
01/14/2012 08:09 AM by
Felice Pollano

@Frans Yes, declared by the authors, of course.But such a declaration requires, if someone disagree, a well explained motivation, and this is what Ayende is trying to do.

Frans Bouma
01/14/2012 10:12 AM by
Frans Bouma

@fellce that's not the point. That some self-proclaimed super-dev is wrong on the internet isn't an incentive to write a series of blogposts to prove that person wrong, as there are a LOT of people wrong on the internet, every day.

The main issue I have with the series of articles is that it's apparently a good thing to simply bash a person's code, as that apparently teaches people something. But that's silly, it only shows someone made bad choices and ended up with a pile of shit. Gee.

If Oren wants to teach people how to do things right, he should write a series of blogposts about how to do it right. As that's 1) more efficient, 2) less bullying and 3) it actually teaches something.

Because there are a million ways to do things the wrong way. Telling your audience this person's code is one of them isn't teaching anything. Well, only if you are first convinced that the way the code bashed to pieces is how you'd write it yourself.

Roland
01/14/2012 10:19 AM by
Roland

Take a look at the upcoming posts. It seems a few blog entries will be devoted to "how-to" instead of "how-not-to".

Ed
01/14/2012 11:04 AM by
Ed

@Frans, in this serie he writes how not to do it and then later he will give examples of a better way to do it. It's a serie.

Also you're not helping calling names like 'self proclaimed super dev'.

You can argue his style but not critizing (bashing in your words) other people's code because you have a populair blog is no argument at all.

Ayende Rahien
01/14/2012 01:55 PM by
Ayende Rahien

Frans, Did you miss the fact that I have... you know, actually written a small amount of posts that actually does go and specifically talks about how and why I think you should write software? The point is to point out bad things, so when you run into them, or use them, you know that they are bad. Then you can go and read about how to do it right.

gunteman
01/14/2012 02:43 PM by
gunteman

We (I) love your many constructive posts, but hate that your approach to show bad practices is almost always to take a public piss at someone else. I no longer think you're doing it to teach, but simply that you get a kick out of the pissing act. It's not uncommon behavior. Most of us do it, in some way or other, but it's seriously reducing the value of your blog, your legacy and, I think, also your business.

Frank
01/14/2012 03:39 PM by
Frank

Learning from mistakes is one of the most effective ways of learning. So I for one am grateful for Ayende taking on this task of pointing out these mistakes.

Tonio
01/14/2012 05:00 PM by
Tonio

@Frans you wrong beacuse the autors of NSK are professional coders that would show me (beginner) the rigth (ideal) code!!! and offer me a book! ayende is rude but real and he yes help me!!

Dan Plaskon
01/14/2012 06:29 PM by
Dan Plaskon

Personally, I think Fabio had it right with his idea of Enhanced Query Objects (http://fabiomaulo.blogspot.com/2010/07/enhanced-query-object.html) - if you need ORM-agnostic support for data access, this approach has little downside.

Dmitry
01/14/2012 07:44 PM by
Dmitry

I wish Ayende would actually take time to create the Macto project instead of making very similar negative posts about various code bases.

Akava
01/15/2012 09:41 AM by
Akava

Morning Ayende, could you tell me what tool did you use to draw the UML? Was it generated by this tool or been written by you?

Thanks

Bob
01/16/2012 01:15 AM by
Bob

@Frans Bouma

Yeah buddy. I've been saying for quite some time now half the post on this big mouths website are simply there to troll for traffic.

Dudes what he is - an assh*le.

Bob
01/16/2012 01:40 AM by
Bob

@Dmitry

Dude, MATCO was just this windbag blowing hard. He said forever he was going to write an end-to-end sample app so everyone would finally see the right way to do it - errrrrr - I mean the Oren way. Whatever.

Dude serves up about a dozen screenshots a year ago and the project goes dark. Typical big mouth. He's too busy reading other peoples code and bashing to finish writing his own.

Ayende Rahien
01/16/2012 07:18 AM by
Ayende Rahien

Akava, I use VS to do that.

Francois Germain
01/16/2012 04:14 PM by
Francois Germain

Ayende, good review. There's one thing I am missing from this post... Did you actually look run this code through a profiler?

Obviously I don't have all the code in front of me but I am sure you are fully aware that entity framework won't load the same PK twice if it's already loaded in the context unless you force it too? Since the unit of work seems to come from the WorkerService, the entity you think is loaded twice should not be... Doesn't make it good anyway, but I think one of your assumption is wrong in this post...

My next question would be, how do you unit test your controllers through a unit test framework without a connection to a database if you do not abstract the DA layer out?

Sean Kearon
01/17/2012 08:17 AM by
Sean Kearon

Right-click method, hit Generate Sequence Diagram. OMG, I never knew that was there. :-))

JohnLiXet
01/19/2012 03:46 PM by
JohnLiXet

@Frans I agreee with you Frans. Why don't you blog about the right way to this? Gosh it cannot be that long since we read "Stored procedures are bad, m'kay?"

Comments have been closed on this topic.