Ayende @ Rahien

Refunds available at head office

Northwind Starter Kit Review: Refactoring to an actual read model

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

In my previous post, I talked about how the CatalogController.Product(id) action was completely ridiculous in the level of abstraction that it used to do its work and promised to show how to do the same work on an actual read model in a much simpler fashion. Here is the code.

image

There are several things that you might note about this code:

  • It is all inline, so it is possible to analyze, optimize and figure out what the hell is going on easily.
  • It doesn’t got to the database to load data that it already has.
  • The code actually does something meaningful.
  • It only do one thing, and it does this elegantly.

This is actually using a read model. By, you know, reading from it, instead of abstracting it.

But there is a problem here, I hear you shout (already reaching for the pitchfork, at least let me finish this post).

Previously, we have hidden the logic of discontinued products and available products behind the following abstraction:

image

Now we are embedding this inside the query itself, what happens if this logic changes? We would now need to go everywhere we used this logic and update it.

Well, yes. Except that there are two mitigating factors.

  • This nice abstraction is never used elsewhere.
  • It is easy to create our own abstraction.

Here is an example on how to do this without adding additional layers of abstractions.

image

Which means that our action method now looks like this:

image

Simple, easy, performant, maintainable.

And it doesn’t make my head hurt or cause me to stay up until 4 AM feeling like an XKCD item.

Comments

Jonas H
01/16/2012 10:14 AM by
Jonas H

Sweet and simple. Thank you for being constructive again :-)

krlm
01/16/2012 10:20 AM by
krlm

I actually do things in a similar way and I like the simplicity of this. Anyway I don't get the .ToList on that subquery. What for? Is it wrong to leave it in a lazy manner?

tobi
01/16/2012 10:24 AM by
tobi

Lazyness makes performance hard to inspect and causes architectural instability. Use when needed. It absolutely the right thing to call .ToList().

Justin A
01/16/2012 10:39 AM by
Justin A

Ayende, I was first introduced to you via Rob Conery's MVC Storefront Pipes and Filters screencast (http://www.asp.net/mvc/videos/mvc-1/aspnet-mvc-storefront/aspnet-mvc-storefront-part-3-pipes-and-filters) -ages- ago.

I so wish Rob warned all viewers to wear a helmet before watching that video because it blew my mind.

Fast forward to today, and I can't get enough of Pipes and Filters and this post once more highlights how relevant it still is and how elegant programming can be when done properly.

Thank you!

Craig
01/16/2012 10:40 AM by
Craig

The problem I have with this is that except for trivial CRUD screens much more logic is generally required. Where are drop down lists populated? What about if along with Products you also want Clients and Sales on this screen, all of a sudden the you have a Controller doing a lot of stuff. And then if you have Save, Delete methods in this controller for the screen before long you have a controller that is 1000 lines long.

tobi
01/16/2012 11:04 AM by
tobi

Craig, why would that be a problem? The code has to be somewhere.

John
01/16/2012 11:07 AM by
John

I really like the WhereProductOnSale() abstractions, any reasion you don't use string.Format for the Title and Keywords?

KoenJ
01/16/2012 11:14 AM by
KoenJ

Thanks for this contribution.

How would you write unit tests for this kind of classes, making explicit use of "Database"? (without actually depending on a real database).

We tend to create an interface for ObjectContext / Database, that is not trying do abstract away any implementation details, but allows us to inject a mock implementation for unit tests.

Regards, Koen

Craig
01/16/2012 11:19 AM by
Craig

tobi, yes it does have to be somewhere. Why don't we just put the whole system in one file! Personally I just dislike opening a file and it says 3632 lines long.

Nick
01/16/2012 11:20 AM by
Nick

It's clearly a lot better than it was before in terms of optimisation, maintainability and comprehension.

We need to be careful for other scenarios, though. Having this kind of logic in the controller may lead some (probably junior) developers thinking validation and business logic can all be lumped in there (Rails? jk)

Then we have an application that is super-quick & 100% efficient getting to the database but crap at everything else.

Much rather this approach with the caveats than the NSK stuff.

The title of tomorrow's post looks very interesting :)

evereq
01/16/2012 11:25 AM by
evereq

Ha, and so every time you want to get list of related products (and in middle size storefront you will probably have many places which want that SAME data), you Ayende will repeat same query again and again??? I don't believe :) Also how you will add caching here, assuming you use EF and want second level cache? In Services approach, it was easy to add caching in the service layer and store all related products altogether in same memcached record... how you will do that, in 100s of Controller actions with same repetitive query code? (please let's don't discuss here output caching - it is just different and for different). Sure if you have one page, which require related products, it is OK to write code you show here and I am SURE we all do this (including people which code you review)! However after you write second time the SAME query to get related products, I am sure you will want to "abstract" it someway, and here you will come back to ICatalogServices one way or another :) Sure you can add many "extensions" to queries, like you did, however it just "helpers", not more! They made your code shorter, prevent some way repetitions, however they are not the same as true service layer.... they are more like extensions to your domain entities and so probably better to embed such logic into domain itself, so domain model will be less anemic etc etc.

I actually found following way to deal with such issues: - initially I use YAGNI principle and write simple queries like you Ayende show here (it is true most simple approach, exactly same like RoR have by default) - later, if see that query repeated for the second time with exactly same parameters, I add it to service layer and reuse it. (and this recommended by many people not only in .NET space, but also in RoR / Grails and many other MVC frameworks if your application GROW from prototype to real one) - I watch carefully to avoid leaky abstractions to not lose performance and do not load same data few times (like was in code your review)...

In Conclusion: I wish Ayende you be more careful when review others code. You just show your opinion, however many of us may not agree or even do it differently and have success. Especially it is true if you review sample applications, which just show WAYS how to do it and smart people study such approaches but select what work best for them in the current situation... And finally if something put you up in the night, others sleep very well with it and back :) So less emotions and more constructive posts like this one will be really better for all of us. Because people in .NET community really tend to overcomplicate things for some reason where it is not required... but it does not mean they are stupid... it just mean they are too smart :). So thanks for your effort to put many of us back to the broken earth ;-), continue your effort but probably with less emotions :)

"Simplicity is always the key to success, unless you do copy / past" (c) EvereQ ;-)

tobi
01/16/2012 11:38 AM by
tobi

evereq, I do it the same way. Only upgrade to a "service layer" if needed. Doing it always is just obsessive behavior.

tarwn
01/16/2012 11:50 AM by
tarwn

Thought I should point out that you made a few changes. You've changed the method signature from int? to int, related products is missing part of the where statement, and you missed the Take(5) on RelatedProducts.

I think I would have still had a single abstraction layer that I called to for the product and then for the relatedproducts. This would have made my intent easier to read and allowed me to run some some basic unit tests against the action to uncover the fact that we aren't handling empty ids (method signature change) and that non-existent id's currently return an exception on the First() call instead of being handled gracefully.

Bjarte Skogøy
01/16/2012 12:17 PM by
Bjarte Skogøy

I like this approach.

One of the major problems of introducing all of theese fancy repository layers is that you often need to eager load different data in different scenarios. Even if the query itself is the same.

Introducing methods for all theese scenarios on a repository seems quite messy to me.

Darren Cauthon
01/16/2012 12:28 PM by
Darren Cauthon

You say:

"Now we are embedding this inside the query itself, what happens if this logic changes? We would now need to go everywhere we used this logic and update it."

The static methods you create on IQueryable might help to solve this problem in your production code, but they're actually going to cause the same problem in your unit tests. Whenever the logic changes, that logic is going to have to be copied to any class that might call the static method. Which, in fact, will keep them from being unit tests because the rules of your product catalog are going to have to be sprinkled to a bunch of tests for classes who are responsible for something else.

That's the thing about static methods -- they can make your code more readable, but they don't absolve the user of the static method's responsibilities. This makes them incredibly difficult to test when they do anything beyond basic, simple things that the developer would have to do anyway.

But given the code as I see it, I doesn't look like unit testing is a high concern for you or the original Northwind authors. At least with their ICatalogControllerWorkerServices it is possible to unit test their code (but they don't), but your refactor that dumps that code into the controller with a static (I think?) reference to Database makes your code impossible to unit test.

If this were my project, I would reject this refactor of yours based on the unit testing problems and the SRP violations.

Darren Cauthon
01/16/2012 12:44 PM by
Darren Cauthon

Well, I will add this one caveat to my statement:

Static methods can make code hard to test, but I think it could be made much easier with tools like TypeMock or its equivalents. They make it possible to override things like static methods or references to make them accessible in the tests.

I never used them in my .Net career, but after comparing and contrasting how testing leads us to create a lot of abstractions and work that might otherwise not be necessary I think they might have a place. I don't want to give up testing, but I also don't want abstractions to take over my system, so maybe those tools are the compromise?

That said, my objection to the controller action on SRP grounds still stands.

Phillip
01/16/2012 12:51 PM by
Phillip

@Craig - Ahh... Partial Views... Use them, then you wont have giant controller actions.

Nick
01/16/2012 12:58 PM by
Nick

@Darren

Not sure how the extension methods cause any problem for the tests? Can you give an example?

cheers

Puneet Bhasin
01/16/2012 01:00 PM by
Puneet Bhasin

Regarding unit testing - If I have complex read queries, how can I test them without hitting the database having predefined data, and then confirming what I got back as right or wrong?

In my code, I have been mocking the database calls, and checking whether the code performs certain validation checks and whether it calls the database. But I am not testing the main intent i.e. correctness of data returned by queries.

Ayende Rahien
01/16/2012 01:04 PM by
Ayende Rahien

Krlm, I do that because I want to make sure that I send the actual materialized items to the view, not queries that may end up being executed. Note that this is a SELECT N+1 in hiding, but because we only get the first one, this is okay.

Ayende Rahien
01/16/2012 01:06 PM by
Ayende Rahien

Craig, In most cases, you would have a dedicated controller for all of those drop down stuff, because you would be using it from multiple locations and filling it with ajax. If you don't, they you either put this in a base controller or a help class, or something like that.

Ayende Rahien
01/16/2012 01:06 PM by
Ayende Rahien

John, I assume that this happens on the server side, so I used simple concat, can be done that way too

Ayende Rahien
01/16/2012 01:07 PM by
Ayende Rahien

KoenJ, I would use an in memory database, easy, simple and it actually works.

Ayende Rahien
01/16/2012 01:12 PM by
Ayende Rahien

Everq, Please don't try to put words in my mouth, or try to take a proposed approach out of context and then try to criticize it. In real world application, the question never comes up. There is no business need to actually show the related products, they are used purely for the UI. I would do all of this in a separate controller action, and wire this on the client side using Ajax. The code is simpler, load time is faster, and you can keep DRY.

I strongly believe in preventing code duplication, but assuming that services is how you do it is wrong.

I would point out that in most applications, repeating a query (beyond a simple get by id) is actually fairly rare. Queries are tailored for specific places, and aren't usually shared.

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

Tarwn, Those changes are somewhat intentional (the int? to int, for example - there is no sense in having a null product here). The Take(5) was a miss, indeed. I don't think that I missed part of the where clause, though.

Ayende Rahien
01/16/2012 01:15 PM by
Ayende Rahien

Darren, This code is easily unit testable: http://ayende.com/blog/3983/nhibernate-unit-testing

Darren Cauthon
01/16/2012 01:37 PM by
Darren Cauthon

Ayende, that's not a unit test. Not a big deal in this case, you can just use a memory database. But what makes it hard to test are the business rules you're placing in the static methods.

You have two static methods:

WhereProductsOnSale (checking IsDiscontinued) WhereProductAvailableForSale (checking UnitsInStock and referencing WhereProductsOnSale)

When you write the tests for this controller action, the example product you have to create must take these two rules into consideration. Your example product will have to have IsDiscontinued == false and UnitsInStock > 0.

But what happens when the rules change, like you mention? What if, say, someone says that in order for a product to be on sale, it must have a price that is greater than $5? Or if units in stock needs to be greater than 1-2 to give room for eventual consistency? You can make that update in one place in your production code, but you then have to copy the new rule across any test for any code that references that static method.

It might not seem like a big deal with this small example, but as soon as things start to get a little complex it's going to become difficult. Unit tests are going to get bigger and start to include seemingly abstract and irrelevant data in them (like setting IsDiscontinued=false and UnitsInStock=1 in a unit test for a view result from a controller action). Sometimes the changes will require changes to some unit tests, but not others, so the tests will start to fail or behave in seemingly weird ways when changes are introduced. And worse -- developer trust in the tests are going to drop as they start to introduce test data and changes to the tests, just-to-get-them-to-go-green.

What makes it even more difficult is that you are building the view model in the controller, which means that as far as I can tell is going to require you to write a few dozen unit tests for that controller action. Which is, of course, going to seem ridiculous, so the developer is probably going to say, "Oh this is so simple, no need to unit test it." And then when another view model is built using the same, if not similar logic, they're going to take the same approach, and before you know it -- game over. The app has poor testing, nobody trusts what tests are there, and everybody's afraid to touch it.

evereq
01/16/2012 01:51 PM by
evereq

Hm... no problema - I don't want to put anything in your mouth and don't see where you found it :)

Regarding "out of context" it seems to me like you actually do this when review app: "The Northwind Starter Kit is a sample application ... and intended to be used as a blueprint when designing and implementing a .NET layered application architecture."...

See here ".NET layered"? This is exactly what guys did - they show Layered application, perhaps more layered than you wish / expect it to be... I.e. they add here "Service Layer" and that layer really require something more than fat controller with queries / queries extensions etc... Perhaps, if they decide to go more simple approach and do not introduce Service Layer, the code will be something like you just show, but it will be less interesting to target auditory :)

See here "sample application"? It means it is sample code for people to study how to do things different and it's not "real world" application for sure.... they show usage of many different ORMs, while in most cases you will just select one etc.

P.S. I have no relation to that project at all... it just seems to me that you look at it in wrong angle, the same way like you just look to my comment (I spend time write it, really and try to be constructive enough etc... will try not to bug you anymore)

Ayende Rahien
01/16/2012 02:03 PM by
Ayende Rahien

Darren, Those methods don't do anything, they merely add filters to the query. You can test that code using in memory, if you really wanted.

And yes, I would have to take this into account, which is great, because I am actually testing not that I am calling the DB, but that the operation is actually doing the right thing given the proper data set. And you seem to think that making a change in production code shouldn't affect the test code? Especially when what you are doing is actually changing the logic that the test is testing? Of course it should cause this change, that is the whole point of it.

I want to do that, because the alternative is to not actually test what the action is doing, but that it goes through the motions.

Managing the test data is something that require a well define structure, but that is easily done.

Why on earth would I want to write dozens of tests for the controllers. You need to check the actual behavior, and then any logic (if statements), and that is about it.

Matt
01/16/2012 02:04 PM by
Matt

I Have to say I disagree with this approach. It puts too much weight on the controllers, and does not allow for re-use of code. I know you say the code is not reused, but in reality it very often is.

I can think of a number of examples: * You have a separate mobi web application to your primary web application in your solution * You want to expose some sort of API with limited access to your data * You have a ServiceHost that forms part of solution for long-running tasks

I am still absolutely convinced a layer of abstraction from your queries is both useful and necessary. For 5 page web pages, yes, your solution will work fine - but for most real-world business applications - I don't think so.

Darren Cauthon
01/16/2012 02:17 PM by
Darren Cauthon

Ayende,

Of course changes to product code should result in changes in the tests (but hopefully the other way around). The question is -- what changes?

If I make a change to one of your static methods and then suddenly every controller test that touches a product suddenly fails, that's a problem. Even if you refactor the test data with some structure, the brittle nature of the tests is still there.

And like everything, the tests are telling you something. You're still duplicating the logic with the static methods, because every controller action is still duplicating work -- they're just duplicating a static method call instead of longer expression.

I think this all really comes down to two things, TDD and the Single Responsibility Principle. Those are the fundamental disagreements between us on this issue. But I won't keep you all day debating this. Thanks for the discussion and the chance to reply. Have a good day! :)

Darren

Phil
01/16/2012 02:19 PM by
Phil

I don't think you filtered the requested product from the related products as the original did.

Me
01/16/2012 02:26 PM by
Me

Are you on drugs?

Matteo Fontana
01/16/2012 02:35 PM by
Matteo Fontana

Oh God! This is the worst solution I've ever seen!

Ok, NSK is probably not so good as it's painted, but this code is even worse.

Puneet Bhasin
01/16/2012 02:38 PM by
Puneet Bhasin

@Matteo - can you please elaborate on the "worst" attribute?

Wayne M
01/16/2012 02:39 PM by
Wayne M

I don't know.. I still feel "uncomfortable" putting what seems to be pure data access code inside a controller action; maybe it's my WebForms background but that stinks to me and evokes bad memories of button "Click" events creating SqlConnection and SqlCommand objects and filling widgets from an untyped DataSet.

It seems to me that there should be a small abstraction layer keeping this code in a separate library, so it could be used elsewhere or kept in isolation. I understand the reasoning behind having the code in the controller itself, but something about it just feels "dirty" to me.

Matteo Fontana
01/16/2012 02:47 PM by
Matteo Fontana

Worst just to talk in an Ayende-like language.

You Cannot reuse this code; You Cannot unit test those two business logics (at my home filtering data to obtain particular significant sub-set of data IS actually BL); Executing that code there's the high probability to load the entire DB in memory

Ayende Rahien
01/16/2012 02:55 PM by
Ayende Rahien

Evereq, I meant, when you assume that I suggest repeating the query over and over again.

Layering requires a reason, and a sample for layering requires a reason for actually doing that. You can teach how to put in an intravenous needle using an orange (to a degree), but trying to explain how to set a bone using an orange is pretty much useless.

And if you wanted to demonstrate the difference between OR/Ms, you wouldn't be trying to wrap and hide them

Marco
01/16/2012 02:55 PM by
Marco

reads may be simple, but they are very important none the less and i still encapsulate them behind an interface. this gives me good SoC and i can still easily maintain and profile each one individually. i just create a new class for my query and use the following api from my controllers/presenters:

Query.Get(new GetTodaysDeal());

SteveR
01/16/2012 02:57 PM by
SteveR

I think it's revealing that people object to this code being in the controller but they'd be fine with it if the exact same query was wrapped in an ICatalogService or whatever.

The reuse argument is weak, especially when you're talking about trying to use that same Service class between different applications like Matt talks about. By doing that you're assuming not only that the querying needs of those different applications is the same on day one (doubtful) but also that the querying needs of those different applications are going to evolve in the same way and at the same rate.

Abstraction isn't about reuse, it's about making code easier to change and coupling several different applications to the same DAO really only guarantees that this class will be the focus of lots of code changes throughout the lifetime of those applications. I've seen this happen and the result is usually a bloated class that tries to serve several different masters, and at that point your SRP is out the window anyway.

tobi
01/16/2012 02:57 PM by
tobi

Matteo, there is not yet a need to reuse it. If the need arises, extract a method.

Hardly any two pages are the same from the standpoint of a database query. There is inherently little reuse.

Ayende Rahien
01/16/2012 02:58 PM by
Ayende Rahien

Matt, Those sort of things are actually very rarely re-used. Taking the mobile app as an example, you don't show recommended products there, because there is no screen real estate to do so. Trying to retro fit things that were meant for one purpose for another is going to cause you a lot of problems, it is easier to just go ahead and do it.

Ayende Rahien
01/16/2012 03:00 PM by
Ayende Rahien

Darren, If you change the static methods, you probably did it for a reason. And that means that everyone who calls them is impacted. The tests should be changed, if this is a change that result it different behavior, and they are testing that the old behavior is there.

Puneet Bhasin
01/16/2012 03:10 PM by
Puneet Bhasin

@Matteo

If you need a certain call to database from more than 1 action, then you would move into a common place. And probably that place is your repository. So, one can argue that using repositories is what it will lead to anyway. However, there are 2 problems:

1) The repository methods will start becoming overloaded when each call using those methods, need a slightly different sorting or prefecthing or something else. And then one would think, it is better to keep the logic simple to avoid a lot of conditional logic. This would be done by splitting the method again.

2) Unit testing - I do not think we are unit testing the correctness of the data anyway. Regardless of where the logic resides. Perhaps testing in rails like way of going to the database is not a bad option. But for databases having large number of tables, setting the data in DB is not an easy thing.

evereq
01/16/2012 03:27 PM by
evereq

"... you Ayende will repeat same query again and again??? I don't believe :)".. see? I don't believe you will do it that way! Sure I know you will not do copy / past etc... So that is why I question that you will follow your own simple approach (i.e. queries in controllers) ones application will grow in size to something like app you review or bigger :) It will be interesting to see your code in such case, really :)

In app you review Layering WAS probably the requirement and that was probably THE "reason" you try to found :) See? :)

Yes, if you want to show most simple WORKING approach, your code is perfect! Really. But I think we really should say BIG thanks to projects like you select for review, because they show more complicated requirements by some sample code and how people attack complicated requirements (sure with some mistakes, completely agree.. no code is perfect :D)... This is exactly what I want to tell when speak about "emotions": we all benefit when you show "mistakes" and the way how you would fix them... however last your sentence "And it doesn’t make my head hurt or cause me to stay up until 4 AM feeling like an XKCD item." is "emotions" in review of others code! People spend time (and probably some money) to write code which go open source and I am sure community (OK, at least myself and few people from comments for sure) benefit from it. :)

Matt
01/16/2012 03:30 PM by
Matt

This is a really interesting discussion.

I actually don't think either approach is truly wrong, but I do think you get more flexibility and a more readable internal API having a layer dedicated to your data retrieval.

@SteveR - I take your point, but to say the data requirements is NOT the same is an odd statement. I have seen many mobi/web splits which use almost EXACTLY the same data - it's just displayed differently due to screen constraints. The UI projects are different, the underlying data is the same. If you want to quibble about one or two extra columns being loaded then I think that's optimisation gone wrong. In the cases where the applications load different data and it's only used once they could use Ayende's approach, but for the majority of queries - yay for the extra layer.

To me, the DRY principal should be king - and I have yet to see a compelling argument to the contrary.

I think this is going to be a situation where we're going to have to agree to disagree!

Wayne M
01/16/2012 03:35 PM by
Wayne M

One thing I'm curious about. What if this code wasn't using an ORM? What if it was just using stored procedures and DataSets?

Is the argument for putting the data access code directly in the controller valid only because of the ORM? Wouldn't there need to be some kind of abstraction layer if the ORM wasn't present, but in a case like this the ORM is abstraction enough?

Ayende Rahien
01/16/2012 03:37 PM by
Ayende Rahien

Wayne, If this was using SP, I would call it out as an improper abstraction. I would much rather have something like Massive or Peta.Poco than that.

NexusSharp
01/16/2012 03:43 PM by
NexusSharp

If you want something reusable use the specification pattern, it was created for it. See http://linqspecs.codeplex.com/ for a possible linq implementation

Wayne M
01/16/2012 03:50 PM by
Wayne M

Thanks, Ayende. I would much rather have Massive or PetaPoco myself (well I would prefer NHibernate of course) but was just curious as at work we are basically forced to use sprocs and DataSets everywhere and trying to use something else gets shot down, but maybe we could use some kind of abstraction layer to avoid having that mess in code-behind.

SteveR
01/16/2012 03:58 PM by
SteveR

@Matt - If you're talking about a mobile web application I would have thought that it could be the same application just using responsive design to deliver a layout optimized for smaller screens. A platform native mobile app might be a different proposition but my experience of those has been that they often differ in their informational requirements quite a lot. Where there is some commonality, like for example a search feature, then that might even deserve to be an entirely separate application in it's own right. In other words more vertical service partition than horizontal DAO service sharing because it promotes better composition.

It's really not that odd that different applications have very different data requirements or different models of the same data source, and having the freedom to allow them diverge in response to business needs and change at different rates is the main point.

Bunter
01/16/2012 05:55 PM by
Bunter

Superb sample to show where to NOT use layering. I can already imagine architecture astronauts jumping up and down realizing that since example contains two business rules, tons of strategies, factories, mappers and injections can be thrown in. All to

a) select a product by ID from allowed set b) add related products from same category in given order c) map query result to a view model

Arguments of reuse: if such piece of code needs to be called twice duplication is already introduced somewhere up the chain.

João Bragança
01/16/2012 06:38 PM by
João Bragança

@Darren, then you should capture that business logic on the command side, i.e. trying to put a product whose cost is less than $5 on sale throws an exception. But that would require a domain that was more than getters and setters.

Reuse? Really how hard would it be to move all these extension methods of IQueryable[of T] to a separate assembly? Now you can reuse it.

Another thing I like about extension methods for this is that they better capture the UL. You can chain them together. You can compose them.

Where else would you build the view model but in the controller? Who knows more about the requirements of the view? The controller or a service behind WCF?

What's more maintainable? 2 lines of code as above or a bunch more scattered throughout multiple assemblies? What ever happened to the simplest possible thing that could work?

Roland
01/16/2012 07:07 PM by
Roland

Compare to the original code:

http://ayende.com/blog/Images/Windows-Live-Writer/6a03ddd05a0846EE/image8.png

hpcd
01/16/2012 11:02 PM by
hpcd

Hi Ayende: I agree that there was way too much abstraction going on in the code you reviewed. However, I think you might have gone too far on the other side. I believe that even you read should be in a service- basically not in the controller. The reason, we can't reuse what you have in a non mvc app. You could have a web app serve different devices, but it is not inconceivable that we want to provide a UI more native to the device and reuse the service and view model.

Case in point: Android UI and IOS UI.

But what you have done is valid-but it breaks reuse scenarios. This is not a theoretical argument- I have seen and been involved in dev for both devices.

Mike
01/17/2012 12:40 AM by
Mike

Considering that your solution is essentially one single LINQ statement, I'm not sure I agree with this statement: "It is all inline, so it is possible to analyze, optimize and figure out what the hell is going on easily."

When giant LINQ queries don't do what you expect, in my experience they're not so easy to debug without breaking them into smaller chunks, etc.

Also on an unrelated note, unless I need the result to be mutable I generally prefer .ToArray() over .ToList(), but that's just personal preference.

Ryan
01/17/2012 01:27 AM by
Ryan

@hpcd - But code reuse across applications is not an immediate up front requirement for most apps. When you need it you can refactor to it. Plus your mobile app might only show a subset of data, or have different pagination requirements, etc. It's futile to try to guess all these up front. It's best to refactor later.

SPATEN
01/17/2012 03:22 AM by
SPATEN

Oren,

Just out of curiosity, what would the Database class look like to enable the Products property as you've shown?

Regards, Stephen

Scott
01/17/2012 04:59 AM by
Scott

Does EF not collapse the expression for and execute the N+1 sub select on RelatedProducts to resolve p.Category?

Dmitry
01/17/2012 06:57 AM by
Dmitry

Ayende, if you were to use a mini ORM that requires an SQL query string or even straight ADO .NET, would you still do it inside the controller?

I do not see why LINQ-to-external data source is any different.

Allan
01/17/2012 08:35 AM by
Allan

Ayende, how would you go about second level caching with your solution?

I think we'ed definitely end up with queries being duplicated unless extension methods or specifications were used from the beginning, I mean working in a team your not going to know someone else has written the query you need using an inline query or your a new starter on a legacy project, whereas repository.GetMeSomething is clear.

But yes I see your point and it is very annoying having to update your Repository and IRepository whenever a new query is needed, more work but I guess most large scale application will need some abstraction, say we had some new requirement which we needed to introduce a web service (This is fairly common) in this case we'ed be trawling through UI code gathering queries we need and placing them in a central location, anyway, or we could be lazy and not refactor existing code and just copy and paste :) If this were a brownfield project I was working on I would be frustrated and wish a repository was used.

Phillip
01/17/2012 08:47 AM by
Phillip

Allan - He's already using output caching, theres no need for 2nd level caching like with NH.

Daniel
01/17/2012 09:03 AM by
Daniel

@Oren/Ayende

Mike said: "When giant LINQ queries don't do what you expect, in my experience they're not so easy to debug without breaking them into smaller chunks,"

I second this. I consider single line linq/fluent style statements as an anti-pattern. "Elegant" - perhaps, as long as it runs. How many different places of a null pointer exception do you see here?

I would break it into 3-4 chunks. Those chunks could be refactored out into individual methods for re-usage once I see the point.

Allan
01/17/2012 10:07 AM by
Allan

Philip - Output caching won't help if I want the cached result elsewhere, i.e. a different view.

Rich
01/17/2012 01:40 PM by
Rich

I think the bigger question is, why the hell are they using an ORM for this type of "read model"?

Karep
01/17/2012 04:58 PM by
Karep

You gotta be kidding that this is clean code? Have you read about Composed method?

Karep
01/17/2012 04:59 PM by
Karep

OutputCache varybyparam none on method that takes parameter. Ok.

Ayende Rahien
01/17/2012 05:33 PM by
Ayende Rahien

Spaten, In this case, just a class that expose NHibernate's session.QueryProduct

Ayende Rahien
01/17/2012 05:34 PM by
Ayende Rahien

Scott, Yes, but we only have 1, so it is okay

Ayende Rahien
01/17/2012 05:34 PM by
Ayende Rahien

dmitry, Yes, I would put the SQL directly in the controller if I was using a mini ORM

Ayende Rahien
01/17/2012 05:36 PM by
Ayende Rahien

Allan, Caching is not a cross cutting concern, and you want to do that in the place with the most context. That is in the outer layers of the system. You can do that with adding a Cache() method to the query on the controller, very easy and simple, or by caching the actual output from the server, so your code doesn't even run.

Ayende Rahien
01/17/2012 05:37 PM by
Ayende Rahien

Daniel, There is no reusing this piece, it is made to order. And while I understand that some people find this uncomfortable, I find this style of code highly readable, even for large amount of code.

Scott
01/17/2012 05:56 PM by
Scott

@Ayende, but before you call .First(), you've called .ToList() when defining 'q'. That won't cause the full query to execute even thought the inner query has dependency on p.Category? Are you positive you have full deferred execution until you call q.First()?

Ayende Rahien
01/17/2012 05:58 PM by
Ayende Rahien

Scott, No, it wouldn't, because the inner query is going to eval only when the outer is executed.

Karep
01/17/2012 06:03 PM by
Karep

When I see code like this I think "Oh the person just learned LINQ".

Karep
01/17/2012 07:54 PM by
Karep

everequ: "I watch carefully to avoid leaky abstractions to not lose performance" I think you are wrong. In order to not loose performance you need leaky abstractions.

Karep
01/17/2012 08:41 PM by
Karep

Regarding dropdowns. I can't imagine how you fill them with separate calls to other controllers. First you risk displaying a page with combos empty for a second or two or worse longer. Also you make more hits to a server instead of just one. From previous posts you seem to take care not to hit db/server more than you need to. But that would be the case when getting data to dropdowns using AJAX.

Ayende Rahien
01/17/2012 09:09 PM by
Ayende Rahien

Karep, Do that in a child actions, then.

Mr Simple
01/18/2012 02:41 PM by
Mr Simple

@evereq

Because people in .NET community really tend to overcomplicate things for some reason where it is not required... but it does not mean they are stupid... it just mean they are too smart :)

Amen brother. You're the kinda guy I hire instead of propellor heads who are more concerned about which pattern they are going to use instead of fixing my problems with simple code. Too often they end up fixing their patterns instead.

Comments have been closed on this topic.