Ayende @ Rahien

It's a girl

Taking a look at S#arp Lite– The MVC parts

This is a review of the S#arp Lite project, the version from Nov 4, 2011.

Okay, after going over all of the rest of the application, let us take a look a the parts that actually do something.

the following are from the CustomerController:

image

It is fairly straight forward, all in all. Of course, the problem is that is isn’t doing much. The moment that it does, we are going to run into problems. Let us move a different controller, ProductController and the Index action:

image

Seems fine, right? Except that in the view…

image

As you can see, we got a Select N+1 here. I’ll admit, I actually had to spend a moment or two to look for it (hint, look for @foreach  in the view, that is usually an indication of a place that requires attention).

The problem is that we really don’t have anything to do about it. If we want to resolve this, we would have to create our own query object to completely encapsulate the query. But all we need is to just add a FetchMany and we are done, except that there is that nasty OR/M abstraction that doesn’t do much except make our life harder.

Comments

Phillip
03/07/2012 10:21 AM by
Phillip

When it comes to C#, I find code formatted like that hard to read. (my opinion)

Tomasz Kubacki
03/07/2012 10:28 AM by
Tomasz Kubacki

All we need to avoid n+1 is .... simpler ORM to make query per view. Can be done easier with any any micro-orm ServiceStack.OrmLite or Massive using raw SQL (which is much widely known and easier to understand than NH or EF magic).

Mark Adamson
03/07/2012 11:29 AM by
Mark Adamson

I may be missing the point.

Could this be easily fixed with a .Include(p => p.Categories) appended on to the controller query. That would force it to download the categories in the original select... I think

Jason Meckley
03/07/2012 11:43 AM by
Jason Meckley

@Thomasz ormlite won't change anything in this case becuase you still have the repository abstraction.

morcs
03/07/2012 11:44 AM by
morcs

@Mark or FetchMany as Ayende points out, but it's the fact that a developer likely won't even be aware of the N + 1 problem that he's raising, I think!

Dooh
03/07/2012 12:12 PM by
Dooh

It's just one of many problems that repositories are producing for sake of advantage of the "easy" ORM switching.

Rémi BOURGAREL
03/07/2012 12:52 PM by
Rémi BOURGAREL

The problem with this war against repository is that you consider querying the database is neither the view or the model.

I don't like repository either because the way you're creating your linq query after GetAll() will change depending on the ORM (for instance some Linq integration will handle the Contains, and some not). But I always create a "ProductDataSource" with a "IEnumerable Get(ProductFilter filter)" method (I don't think it's a repository), and if I need to I create a ProductOutputFilter that'll say if I need to load the Categories or not.

ORM switching is not a legend, It's interesting to be able to switch when you want to try something new (with my method I recently moved from plain SQL to NH) or to leave an abandoned framework.

magellings
03/07/2012 01:54 PM by
magellings

Can't you just set batch mode for selects on in your session factory configuration? All in all so far, I am liking the original S#arp better than Lite.

Demis Bellot
03/07/2012 01:55 PM by
Demis Bellot

@JasonMeckley

Yes using a Micro ORM like OrmLite does help prevent this as you're only returning POCO models (i.e. not IQueryables/ORM DCs/etc that supports additional querying) so everything needs to be pre-fetched in the repository before hand making it impossible to perform an N+1 query outside of the repository.

Micro ORMs remove the "hidden abstraction" you get with Heavy ORMs and what you're left with is very much a "whay you query" is "what you get".

Ayende Rahien
03/07/2012 01:56 PM by
Ayende Rahien

magellings, Oh, I can do that, and have this happen everywhere ? That seems to be too drastic a step

magellings
03/07/2012 02:02 PM by
magellings

HasMany(x => x.Children).AsSet().SetAttribute("batch-size", "20")

???

Tungano
03/07/2012 02:11 PM by
Tungano

I don't like passing domain objects to views. For each view I create a 'view model' where I either expand or flatten some data.

A view model would in this case have a categories collection that if of type IEnumerable since that is all what the view is rendering.

I let the controller project data on the 'view model'. I prefer to not have IQueryable on repositories, avoid All() if possible.

In some cases you grab a bit too much data only to filter it out in-memory. If performance suffers I could create a specialized more performant query somewhere. But then again if I am tailoring for the incompetent: they would not call it would they?

I see little value in these 'color between the lines' projects. The architecture's value lays in dealing with the incompetent who perhaps shouldn't be touching your codebase to begin with. I prefer working with people who can draw.

Tungano
03/07/2012 02:13 PM by
Tungano

correction: IEnumerable of type string

...it ate my angle brackets

Felice Pollano
03/07/2012 02:59 PM by
Felice Pollano

I don't know if it is just left apart in the example, but the GetAll() has no parameters for limiting the result set. That is something thet makes me always angry;)

Mohamed RAHIM
03/07/2012 03:37 PM by
Mohamed RAHIM

NH FetchMany Or EF Include is the only way!

Rajeesh
03/07/2012 04:49 PM by
Rajeesh

Thanks ayende. I have been explain about this issue to my friend couple of days back. My suggestion was to flatten the model to simple plain view model depending on the scenario. So that view won't be generating the database queries.

I will refer this post him.

Kiliman
03/07/2012 06:44 PM by
Kiliman

@Tungano, Great quote!

"I see little value in these 'color between the lines' projects. The architecture's value lays in dealing with the incompetent who perhaps shouldn't be touching your codebase to begin with. I prefer working with people who can draw."

Karep
03/07/2012 08:28 PM by
Karep

Oh c'mon! Author is creating a framework and he doesn't even know what SELECT N + 1 is!? I think I have to create my own framework soon.

Dmitry
03/07/2012 10:48 PM by
Dmitry

Is there anything in the framework that actually simplifies development? In all the posts I have not seen anything that saves or improves code that I could write using NHibernate and Automapper directly.

gregmac
03/08/2012 07:15 PM by
gregmac

I agree with @DemisBellot here. Using POCO (rather than EF/NH/whatever entities) absolutely prevents N+1 and also forces you to think about data access. Too many people use NH or EF without understanding how they work (and there is a lot to understand), and as a result their code is full of lazy loads and other dumb queries. Both good and bad, in a small app with low traffic/small DB, this doesn't really matter much. As it grows, this starts to matter a lot.

I have a data access "abstraction" I guess (in that I put my data access code in one place, usually its own project to enforce low coupling and awareness of the API surface area if nothing else), and then build a simple API to expose the data operations I need, which all interact using POCO objects. As the db schema evolves or I want to enforce rules on top, there is one place to do that. I definitely don't want to see WHERE-type clauses in front-end application code.

If the app has a lot of business rules, then there is another layer between app and data access to handle that, though really I am less fussed with if "business rules" exist in the DB (stored procs/triggers), data access (WHERE logic) or code layer, than that they do NOT exist in front-end. This is especially when there are multiple parts of the UI or other apps (web API, scheduled tasks daemon, etc) that do the same interactions.

Comments have been closed on this topic.