Ayende @ Rahien

Refunds available at head office

Taking a look at S#arp Lite, Part III–tasks

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

So far, we have gone over the general structure and the domain. Next on the list is going over the tasks project. No idea what this is. I would expect that to be some sort of long running, background tasks, but haven’t checked it yet.

Unfortunately, this isn’t the case. Tasks seems to be about another name for DAL. And to make matter worse, this is a DAL on top or a Repository on top of an OR/M.

And as if that wasn’t enough to put my teeth on edge, we got some really strange things going on there. Let us see how it goes.

image

Basically, a CudTask seems to be all about translating from the data model (I intentionally don’t call it domain model) to the view model. I spoke about the issues with repositories many times before, so I’ll suffice with saying that this is still wasteful and serve no real purpose and be done with it.

This TransferFormValuesTo() is a strange beast, and it took me a while to figure out what is going on here. let us look in the parent class to figure out what is going on there.

image

Let me count the things that are wrong here.

First, we have this IsTransient() method, why do we have that? All we need to do is just to call SaveOrUpdate and it will do it for us. Then the rest of the method sunk in.

The way this system works, you are going to have two instances of every entity that you load (not really true, by the way, because you have leakage for references, which must cause some really interesting bugs). One instance is the one that is managed by NHibernate, dealing with lazy loading, change management, etc. The second is the value that isn’t managed by NHibernate. I assume that this is an instance that you get when you bind the entity view the action parameters.

NHibernate contains explicit support for handling that (session.Merge), and that support is there for bad applications. You shouldn’t be doing things this way. Extend the model binder so it would load the entity from NHibernate and bind to that instance directly. You wouldn’t have to worry about all of this code, it would just work.

For that matter, the same goes for validation as well, you can push that into NHibernate as a listener. So all of this code just goes away, poof!

And then there is the Delete method:

image

I mean, is there a rule that says “developers should discard error information as soon as possible, because it is useless.” I mean, the next step is to see C# code littered with things like:

catch(Exception e)
{
   delete e; // early release for the memory held by the exception
}

The one good thing that I can say about the CudTasks is that at least they are explicit about not handling reads, and that reads seems to be handled properly so far (but I haven’t looked at the actual code yet).

Comments

Apostol
03/06/2012 10:28 AM by
Apostol

Made me sad to see this. It seems the people that wrote this were not aware of some of the features of ORM-s and NHibernate in particular. I wish more people do research about their tools before they code.

Owen
03/06/2012 10:59 AM by
Owen

While I agree the code is over the top and unnecessary, binding to the data/domain model in the controller is not the way to go.

I'd much rather write a view model specific to each controller action than a custom binder. Validation is a lot simpler (FluentValidation or DataAnnotations) as validating a form isn't always the same as validating the NHibernate model.

In most cases AutoMapper can be used to update the NHibernate model from the view model instead of writing line after line of code like: a.Name = b.Name

morcs
03/06/2012 12:16 PM by
morcs

@Owen I generally agree but often come across these issues:

  1. The view model usually contains more properties than I actually bind to in the post action (additional lookups etc.). For this reason I often have a "Post Model" with those fields I actually want to update, and a "View Model" which inherits from the Post Model and adds any view/lookup data fields. Obviously usually we want to avoid inheritance but so far this has worked out pretty well for me.

The alternative to using inheritance would be to have two separate classes with duplicate properties, one for view and one for form/post, but I would need to make sure I copied any validation attributes between both to make sure that client-side and server-side validation still worked. Using inheritance means the validation is always shared across both cases.

  1. I don't like AutoMapper. It means that if I rename a property on my entity, I must remember to rename it on my view (post) model to match - or all of a sudden my forms will stop updating that field without warning. (Unless I have a full suite of acceptance tests to cover that, which I guess is the ideal!).
Dmitry
03/06/2012 02:18 PM by
Dmitry

@morcs,

AutoMapper has an assert method to prevent such problems without having to unit test each property.

morcs
03/06/2012 02:31 PM by
morcs

@Dmitry Thanks :)

Found it here: http://automapper.codeplex.com/wikipage?title=Configuration%20Validation

Pavel Penkov
03/06/2012 06:57 PM by
Pavel Penkov

I really don't understand why people keep creating these things on top of ORMs context. It's a Repository and Unit Of Work wrapped in one package. You don't need wrappers for CRUD, the basic context does those things just fine. You need wrappers if there's some real work going on.

Pavel Penkov
03/06/2012 07:09 PM by
Pavel Penkov

Oh, those Haskell-styled return values. Anyone else thinks they are useless without at least pattern matching?

Wayne M
03/06/2012 07:20 PM by
Wayne M

I've seen the "Tasks" pattern before, it's basically what the rest of us would know as "Services", the layer around the Repository that everything calls into (and which subsequently just wraps the calls to the Repository).

Thing to remember is this was the "assumed" way of doing data access in .NET before we heard of CQRS and before the geniuses like Ayende started not using Repositories. Most of us learned this style of code from various presentations and screencasts and books which did just this: Service wrapping a Repository wrapping an ORM. I know for a fact I learned this style from watching the MVC Storefront series.

Pavel Penkov
03/06/2012 07:32 PM by
Pavel Penkov

@Wayne Services should do something meaningful, like sending an email, printing a receipt or saving an audit trail. If you use service to wrap a single CRUD operation you're wasting your and more importantly your colleagues time. I think it has something to do with good IDEs which allow programmer to navigate the code real quick. If you're using vim in 80*25 you just can't get your head around all this stuff.

Dmitry
03/06/2012 07:47 PM by
Dmitry

I have always used a single generic repository before LINQ-to-ORM/IQueryable was available in .NET. Now you can just use the Query<> method most ORMs provide.

I never understood the reason of making a repository-per-type or even per-aggregate root with custom methods when you have services.

Željko
03/06/2012 08:28 PM by
Željko

If Sharp Lite is a simplified version of the full Sharp architecture, then I'm really glad I never took a look at it.This really is way to much for my stomach.

gunteman
03/06/2012 11:11 PM by
gunteman

Github just learned the hard way ( http://arstechnica.com/business/news/2012/03/hacker-commandeers-github-to-prove-vuln-in-ruby.ars ) that model binding directly to persisted entities can be really dangerous, and I prefer to have middle man DTOs. Binding directly to the entities solves the Microsoft demo-ability issue, but it's just not worth it.

Renaming the service layer "Tasks" is a really good idea. It's what they are, and the name doesn't imply anything about "backgrounding" etc.

However, reinventing CRUD for the gazillionth time is just...yuck.

Daniel Lang
03/07/2012 03:35 AM by
Daniel Lang

I can't really believe you would actually want to call Session.Load inside a custom model-binder. In terms of readability of code, this really sucks as it hides the very important database-request from the one reviewing the code. If I look at an action, I want to understand what is going on there, without digging through the solution explorer.

Jean
03/07/2012 10:20 AM by
Jean

Seeing from the comments and the mindless followers, I start to think these post series are pointless. If you pick one piece of code and take it out of context, in most cases you'll find it just unecessary and would like to make it simpler and change it completely. However when you start asking the right questions and at least try to understand what were the problems the original developer was trying to solve, it would probably make more sense. Unfortunately the author is not here to defend himself.

@Apostol: Check out who's behind the project before making a fool out of yourself. @Pavel: Making things always simpler is a good thing but there are reasons devs don't want to depend on third party frameworks, hence the intermediary layers. Simple design is good for simple tools, some of us have to deal with enterprise application designs and last time changes.

Thanks to @gunteman who understands the problem. It is indeed really bad practice to bind data to your mapped entity for the reasons he has mentioned.

@Ayende, come on, that last comment about not capturing the exception was just unfair. If you want to capture the exception, then fine update the source code and pass it to the ActionConfirmation or just log it. Now whether this puts the whole design into question is another matter.

Sorry for the tone, but today I was not in the mood to deal with unfair and generalization comments "Oh I don't like that line there, I will never use this project ever!". For the rest of us who wants to understand the architecture, check here http://bit.ly/w0fQxv

Ryan
03/07/2012 05:02 PM by
Ryan

@gunteman Actually "Task" is already used in the .NET framework to signify an asynchronous process. http://msdn.microsoft.com/en-us/library/System.Threading.Tasks.Task.aspx

@Jean I've found it's easier to make last minute changes when you have less code. Just because you work for an "enterprise" doesn't mean you need to use enterprise architecture. Simpler is better. Pick and ORM or doc db, learn it well, then use Commands, Queries, and Tasks like Ayende has said before. You've just covered 95% of LOB apps without blowing out the complexity. If you need durability, you can get a long way with a persistent messaging framework like NSB.

Luke Winikates
03/07/2012 05:07 PM by
Luke Winikates

@guntenman That's an overly broad characterization of what happened with github (IMO). They did have the Rails equivalent of a DTO (a dictionary of property names and values), but they forgot/didn't bother to do the equivalent of saying "oh, but the DTO should never have an 'IsAdmin' property and if someone tries to pass one you should ignore it." And then they did the equivalent of user = AutoMapper.Map(dto); user.Save();

To the broader topic-- in ASP.NET MVC, an explicit "ViewModel"/"DTO" layer does have the effect of forcing you to think about which properties you really want to modify after the POST action in each unique context. It strikes me as a necessity.

OTOH, sure, AutoMapper is not hard to use, but it is one more piece of infrastructure that you have to test and maintain, just. to. do. CRUD. That's more than a "shucks, can't do the gee-whiz 5-minute demo" problem. That's a "why do I have to write so much code that's not about the domain of my application?" problem.

Steve Sheldon
03/07/2012 08:46 PM by
Steve Sheldon

I like these posts, they make me think.

I haven't figured out where this pattern came from, but in several companies I've encountered code where the data access layer was called an Object Factory. In fact I was talking to a coworker one day and trying to describe the repository pattern and he said "Isn't that an object factory?"

I just found it surprising as I've never seen any MSDN posts or examples like that which use that approach, so I'm not sure where people are picking it up. Maybe it came out of the Java world?

Jean
03/07/2012 09:29 PM by
Jean

@Ryan By enterprise architecture I meant proper separation of concerns and responsibilities. Like most of the commenters think here is that a Service Layer or any kind of abstraction of the ORM is pointless. I completely disagree. Having a Service Layer is crucial to control/centralize access (auditing/authorization/validation) of your entities across multiple front ends/tiers. When I see a controller accessing directly a repository I feel the developer just doesn't care about future development.

Sure, some time you will end up with a service method with just one line calling directly your repository, but this is for consistency ! If you decide every call to your repository should pass first by the service you're not going to break the design and inject your repo in the controller just because it saves you one line. Discipline always pays off.

The other day I had to implement a new authorization mechanism in a large system. So basically, all GetById, GetBySomething had now to also inject the User info. Let me tell you that if there was no Service layer I would have been completely screwed and it would have taken me days to make changes across ALL presentation layers. Obviously if there was one single controller which had a repository injected directly it would have made the implementation much harder but thank god I work with conscientious and disciplined developers.

João Bragança
03/07/2012 10:01 PM by
João Bragança

Never mind that there are no actual entities in this project but whatever. These 'entities' have no behavior. It's just a read model. There's 0 reason to make this so complex.

Ryarn
03/08/2012 12:52 AM by
Ryarn

@Jean If I only need a web front end at first, why am I coding for additional front ends now? How will I know that my other endpoints will need exactly the same information.

As far as integrating user authentication/authorization, if you have proper separation of concerns, you would have something like Context.CurrentUser which can be set at the start of a request by the appropriate authentication module. Then no matter how you do authentication, everything else just sees Context.UserPrincipal. If you have authorization built into many queries and you are using LINQ then you could just make a FilterForCurrentUser() extension method that keeps this logic in one spot.

Depending on the complexity of your query, NHibernate supports the notion of a global filter. In EF you can easily provide a wrapper around the context to do filtering that does not hide the actual context like the repository pattern does. Same goes for mongo and raven.

Jean
03/08/2012 10:47 AM by
Jean

@Ryarn Comparing the small cost of adding a Service layer from the beginning and refactoring a HUGE base code, I can see what wins in terms of reusability and speed of development. If your client asks you to implement a new functionality (outside web scope) and it needs to be integrated in your current system, having the ability to plug in your service layer without any worries of inconsistency (security hole,etc..) is priceless in the eyes of your client.

gunteman
03/08/2012 01:38 PM by
gunteman

@Luke

One extra level of indirection, but still exactly the same thing. Posted values were blindly bound to the persisted entities.

But I think we agree on the actual issue :)

Comments have been closed on this topic.