Ayende @ Rahien

It's a girl

The wages of sin: Inverse 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 Smile), 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.

image

Which is then called from CurrencyService:

image

Which is then called…

image

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.

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Frank Quednau
03/23/2011 01:41 PM by
Frank Quednau

As with every feature of moderate complexity, guidance as to how it is used is needed, fortunately you already wrote about it:

www.google.ch/search

R
03/23/2011 02:09 PM by
R

Send them a patch and stop being toxic

Jason Meckley
03/23/2011 02:23 PM by
Jason Meckley

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.

LukeB
03/23/2011 03:49 PM by
LukeB

I'm always amazed at the thin skin of software developers. Criticism and opinion are not "bashing" and are not being "toxic".

stu
03/23/2011 04:38 PM by
stu

Please explain how the ToList() breaks Future <t().

This should be the point of your post. Write to be understood.

Joe
03/23/2011 04:40 PM by
Joe

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.

LukeB
03/23/2011 05:42 PM by
LukeB

"Please explain how the ToList() breaks Future()."

ToList() immediately executes a linq statement.

Dmitry
03/23/2011 05:44 PM by
Dmitry

@stu,

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.

Chris Marisic
03/23/2011 06:31 PM by
Chris Marisic

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.

throwspoop
03/23/2011 08:12 PM by
throwspoop

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?

John Farrell
03/23/2011 08:48 PM by
John Farrell

@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?

gunteman
03/23/2011 08:49 PM by
gunteman

@LukeB

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...

Alex Simkin
03/23/2011 09:23 PM by
Alex Simkin

@John Farrel - Yeah, that will teach him.

Steve Py
03/23/2011 11:03 PM by
Steve Py

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.

Ayende Rahien
03/23/2011 11:18 PM by
Ayende Rahien

@throwspoop,

Ad revenue? Seriously?

The blog isn't even paying for itself in ads

StanK
03/24/2011 12:32 AM by
StanK

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!

Jay Douglass
03/24/2011 12:45 AM by
Jay Douglass

Keep it coming Ayende. For too long the repository pattern has passed as conventional wisdom.

Belvasis
03/24/2011 12:49 AM by
Belvasis

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? :-(

BoB
03/24/2011 01:48 AM by
BoB

@Joe

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.

NC
03/24/2011 03:21 AM by
NC

@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.

SBG
03/24/2011 04:33 AM by
SBG

No doubt. Go write something amazing and stop being a douche.

Paul
03/24/2011 11:27 AM by
Paul

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.

Belvasis
03/24/2011 12:53 PM by
Belvasis

@Paul

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?

John Chapman
03/24/2011 01:18 PM by
John Chapman

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)

James
03/24/2011 01:39 PM by
James

@Paul

Actually I have un subscribed. This is hardly constructive criticism. Just hateful crap.

Shawn
03/24/2011 06:24 PM by
Shawn

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.

Chris Marisic
03/25/2011 05:42 PM by
Chris Marisic
@John Farrell
  
  
You clearly didn't actually read my article then.  Because by writing this code once I never need to implement a repository it's all handled through StructureMap and Inversion of Control
  
  
By merely putting a dependency on IRepository
<any into my constructors I immediately get access to Get/Save/Delete and to Query. I have to only create discrete queries ever and never waste time writing/copying boiler plate repository code that most people get into.
>
Rafiki
03/26/2011 09:29 PM by
Rafiki

@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?

Comments have been closed on this topic.