http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660@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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment79http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment79Wed, 18 Jan 2012 14:41:38 GMTKarep, Do that in a child actions, then. http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment78http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment78Tue, 17 Jan 2012 21:09:37 GMTRegarding 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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment77http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment77Tue, 17 Jan 2012 20:41:28 GMTeverequ: "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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment76http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment76Tue, 17 Jan 2012 19:54:39 GMTWhen I see code like this I think "Oh the person just learned LINQ".http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment75http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment75Tue, 17 Jan 2012 18:03:31 GMTScott, No, it wouldn't, because the inner query is going to eval only when the outer is executed.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment74http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment74Tue, 17 Jan 2012 17:58:04 GMT@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()? http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment73http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment73Tue, 17 Jan 2012 17:56:42 GMTDaniel, 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. http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment72http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment72Tue, 17 Jan 2012 17:37:34 GMTAllan, 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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment71http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment71Tue, 17 Jan 2012 17:36:33 GMTdmitry, Yes, I would put the SQL directly in the controller if I was using a mini ORMhttp://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment70http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment70Tue, 17 Jan 2012 17:34:34 GMTScott, Yes, but we only have 1, so it is okayhttp://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment69http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment69Tue, 17 Jan 2012 17:34:11 GMTSpaten, In this case, just a class that expose NHibernate's session.Query[Product]()http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment68http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment68Tue, 17 Jan 2012 17:33:58 GMTOutputCache varybyparam none on method that takes parameter. Ok.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment67http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment67Tue, 17 Jan 2012 16:59:46 GMTYou gotta be kidding that this is clean code? Have you read about Composed method?http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment66http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment66Tue, 17 Jan 2012 16:58:30 GMTI think the bigger question is, why the hell are they using an ORM for this type of "read model"?http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment65http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment65Tue, 17 Jan 2012 13:40:43 GMTPhilip - Output caching won't help if I want the cached result elsewhere, i.e. a different view.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment64http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment64Tue, 17 Jan 2012 10:07:23 GMT@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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment63http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment63Tue, 17 Jan 2012 09:03:18 GMTAllan - He's already using output caching, theres no need for 2nd level caching like with NH.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment62http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment62Tue, 17 Jan 2012 08:47:18 GMTAyende, 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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment61http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment61Tue, 17 Jan 2012 08:35:16 GMTAyende, 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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment60http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment60Tue, 17 Jan 2012 06:57:23 GMTDoes EF not collapse the expression for and execute the N+1 sub select on RelatedProducts to resolve p.Category?http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment59http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment59Tue, 17 Jan 2012 04:59:27 GMTOren, Just out of curiosity, what would the Database class look like to enable the Products property as you've shown? Regards, Stephen http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment58http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment58Tue, 17 Jan 2012 03:22:52 GMT@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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment57http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment57Tue, 17 Jan 2012 01:27:08 GMTConsidering 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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment56http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment56Tue, 17 Jan 2012 00:40:01 GMTHi 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. http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment55http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment55Mon, 16 Jan 2012 23:02:18 GMTCompare to the original code: http://ayende.com/blog/Images/Windows-Live-Writer/6a03ddd05a08_46EE/image_8.pnghttp://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment54http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment54Mon, 16 Jan 2012 19:07:27 GMT@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?http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment53http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment53Mon, 16 Jan 2012 18:38:48 GMTSuperb 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. http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment52http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment52Mon, 16 Jan 2012 17:55:58 GMT@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. http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment51http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment51Mon, 16 Jan 2012 15:58:19 GMTThanks, 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.http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment50http://ayende.com/153153/northwind-starter-kit-review-refactoring-to-an-actual-read-model#comment50Mon, 16 Jan 2012 15:50:02 GMT