The wages of sinInverse leaky abstractions
This time, this is a review of the Sharp Commerce application. Again, I have stumbled upon the application by pure chance, and I have very little notion about who wrote it.
As you might have noticed, I am pretty big fan of the NHibernate Futures feature (might be because I wrote it ), so I was encouraged when I saw it used in the project. It can drastically help the performance of the system.
Except, that then I saw how it is being used.
Which is then called from CurrencyService:
Which is then called…
Do you see that? We take the nice future query that we have, and turn it into a standard query, blithely crashing any chance to actually optimize the data access of the system.
The problem is that from the service API, there is literally nothing to tell you that you need to order your calls so all the data access happens up front, and it is easy to make mistakes such as this. The problem is that there is meaning lost at any additional abstraction layer, and pretty soon whatever good intention you had when you wrote a particular layer, 7 layers removed, you can’t really remember what is the best way to approach something.
More posts in "The wages of sin" series:
- (24 Mar 2011) Hit that database one more time…
- (23 Mar 2011) Inverse leaky abstractions
- (22 Mar 2011) Proper and improper usage of abstracting an OR/M
- (21 Mar 2011) Re-creating the Stored Procedure API in C#
- (18 Mar 2011) Over architecture in the real world
As with every feature of moderate complexity, guidance as to how it is used is needed, fortunately you already wrote about it:
Send them a patch and stop being toxic
I understand batched reads. I understand over-architecture. But with this snippet of code I fail to see the problem with the use of Future.ToList(). if this is the only db call then it makes sense to call ToList() at this point. If the remaining block of code had additional Future.ToList() calls, then yes, the benefit of batching is lost.
I'm always amazed at the thin skin of software developers. Criticism and opinion are not "bashing" and are not being "toxic".
Please explain how the ToList() breaks Future <t().
This should be the point of your post. Write to be understood.
I agree with Frank though, if you engage in criticism its helpful to those reading the blog and who are learning to know what the recommended solution is.
Unless of course you have no interest in pointing those who are learning to some helpful reading.
"Please explain how the ToList() breaks Future()."
ToList() immediately executes a linq statement.
Let's say you have another method, GetEnabledDiscounts() that returns a future collection.
If you do
var a = ...GetEnabledCurrencies().ToList();
var b = ...GetEnabledDiscounts().ToList();
there will be 2 separate db calls. The right way to do this is:
var a = ... GetEnabledCurrencies();
var b = ....GetEnabledDiscounts();
var aa = a.ToList();
var bb = b.ToList();
This way the 2 queries will be batched. However, there is no intuitive way to realize this with the encapsulated service objects.
I completely agree that inverse of leaky abstractions is more nefarious than leaky abstractions themselves.
This has lead me to view all people who implement the repository pattern using a base class to have fallen victim to it already.
To have a repository be a dependency through inheritance is the ultimate of giving up since you completely through all aggregation out of the window.
You can't write a CarInsuranceRepository, you need to write a CarRepository, an InsuranceRepostiroy, a StateRepository and god knows how many other concrete class implementations before you can do even the most basic business functionality.
My post is now a year old on this topic but I think it's as relevant as ever for any DAL. dotnetchris.wordpress.com/.../creating-a-common... as the post says you can mostly ignore the Conversation objects they're wrappers for the session that i use to complete the sessions in an AOP manner.
Do you agree with me that my solution is a very proper use of abstraction?
My pattern doesn't "lose features" the way many abstraction layers do, or that you end up with abstractions that completely obfuscate things you're trying to do like the exact point of this post that the future usage was completely lost due to the Controller's usage of the repository layers. With my code you could create a discrete repository that aggregates all of the calls you need to satisfy a demand and keep all of the future aspects behind the repository.
The point of these posts is to create controversy (interest, etc) so that his site gets more traffic and earns more ad revenue, and most importantly, sells more licenses. Duh...seriously, who gives a flying F if this app is written poorly or not?
@Chris Marisic - What abstraction? You may as well call that "NhibernateFacade" as its certainly not a repository.
IMHO the code you've linked to is the perfect example of what Ayende has been talking about. Its 100% useless passthrough code that abstracts nothing. Whats the point of even writing that class?
Repeated criticism and opinion, with no trace of humility or constructive suggestions, may not be bashing, but it's toxic and IMHO it constantly clouds Ayende's undisputable brilliance. More daily WOW than DailyWTF would be nice. Quite possibly also for Ayende's business...
@John Farrel - Yeah, that will teach him.
If anything, this should demonstrate that at least one of NHibernate's best practice paths is fraught with little land-mines. If people are stumbling on them and blowing off limbs then it's pretty clear that information about the best course using futures is missing or gotten foggy in the mist of time.
Ayende's comments are rather toxic, most likely just a reflection of his mood at the time of writing; but it would be good to see links to the correct implementation and pitfalls to watch out for regarding the topic. Rather than just pointing out people's stupid little mistakes, please point everyone to information to make the correct assumptions.
Ad revenue? Seriously?
The blog isn't even paying for itself in ads
Everyone keeps asking to see the 'correct' way to do things - I think that the point is that you don't need to abstract away NHibernate with a repository, just use the session directly in your controller (or whatever you are using). There's no 'correct' code to show - as presumably you already know how to use an NHibernate session!
Keep it coming Ayende. For too long the repository pattern has passed as conventional wisdom.
I agree with gunteman...I'm afraid the time of daily WOW's is long gone. In the past this was a blog where one could learn something and got some new ideas. But now...where are the clues, solutions, suggestions or brilliant ideas? :-(
It's always the same around here. You never get any answers as Ayende is just trolling to get you to one of the $3000 seminars held in London or Tokyo or some other joint that cost $6000 to get to.
@Belvasis - I agree, there's a massive archive of great posts. But over the last year the quality of posts on this blog has diminished.
No doubt. Go write something amazing and stop being a douche.
Seriously people if it bothers you that much just unsubscribe.
As far as I'm concerned these posts just show the dangers in blindly following a patterns and practices approach to development rather than concentrating on the problem in hand. There are plenty of other blogs out there that would have you event sourcing and DDDing your way to a death march on every project. I think Ayende's just trying to bring some balance to that.
It does not matter if it bothers me or not. And I also don't want to blindly follow any pattern or practice. But after a lot of blogs about "How you should not do it" I simply think it is time for ONE "Here is how I would do it" blog, don't you think?
I'm sorry, but I fail to see the egregiousness of using futures in this scenario. I see that in this particular usage scenario, Future is providing no benefit, but it is it causing a problem? Does this scenario perform any worse than it would if we just called List() instead of returning the future?
If not, returning the future seems like a good choice. If the consumer was in fact making multiple calls Future would provide a benefit, and in this case where they are not making additional calls it works the same as it would otherwise.
I realize that it may be harder to see this due to the abstraction. And if they had multiple calls with .ToList() it would defeat the purpose. However, in most cases people don't actually need a List() and can bind the IEnumerable anyway. So I would guess that more often than not consumers with multiple queries would benefit here.
While abstractions may provide simple implementations in certain cases, I do not believe that simple abstractions mean abstractions provide no value. I personally do not want to tightly couple my controllers to NHiberante sessions.
I realize that removing all abstractions can result in an opportunity to achieve better performance as all of the tools of the underlying framework are available to you.
However, that's a trade off I make for clear separation. C# is an abstraction, where I could potentially achieve better performance by properly tuning C++ for example, but I don't since that abstraction provides value to me and the performance penalty is deemed worth it.
I think it's ok to make the same decision on separating persistence mechanisms from UI elements (such as controllers/presenters/viewmodels)
Actually I have un subscribed. This is hardly constructive criticism. Just hateful crap.
I think the point here is that you should challenge your assumptions about how you are designing your software. I don't need code to copy and paste so I can get the next set of received wisdom. I need to be reminded to challenge these assumptions so I can figure out what's right for the software I am designing.
If you're taking this as toxic or whatever, I think you need to consider why you have such a deeply personal investment in this design pattern, or any design pattern, for that matter.
This series has been really valuable, but I'm not even writing code that touches a database at the moment.
@Ayende. You know, MultiCriteriaImpl's code is HORRIBLE. All those array index manipulations make me cry every time I see them.
How on Earth is it possible that you wrote this mess?
Infrastructure code is usually complex, especially when you are working at a low level.
But if you want to talk about horrible code, take a look here: