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.

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.

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:

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