Jose was kind enough to post a review of my sample Effectus application. This post is a reply to that.
The first thing that Jose didn’t like was the fact that I didn’t put an abstraction layer in front of NHibernate’s usage in my application. There are several reasons for that, the first, and simplest, is that I was trying to explain how to use NHibernate, and for that, I wanted to deal with the lowest common denominator, not show off a particular wrapper implementation. Showing NHibernate usage directly means that even if you are using another library with it, you would still be able to take advantage of the information that I give you.
Second, and quite important, is that by using NHibernate directly I can take advantage of NHibernate features explicitly meant to support certain scenarios, but which are not likely to be expose when libraries wrap NHibernate. A good example of that is in Jose’s sample code, which make use of an ISession instead of IStatelessSession to load the data for the main screen. As explained in the article, the difference between the two is important, and in the context where it is used it introduce what is effectively a memory leak into Jose’s implementation, as well as the chance for some really interesting errors down the road if the session will run into an error.
Third, Jose bring up the following:
This presenter is GLUED to NHibernate. And this means for instance that you can’t test it without NHibernate, and this means that you can’t change your persistence layer.
Yes, and that is intentional. That is the very task that those presenters are for. Trying to abstract that away just means that I put an additional layer of abstraction that does absolutely nothing. For that matter, let us look at Jose’s implementation using an abstraction layer compared to mine.
Here is my implementation:
private void LoadPage(int page)
{
using (var tx = StatelessSession.BeginTransaction())
{
var actions = StatelessSession.CreateCriteria<ToDoAction>()
.SetFirstResult(page * PageSize)
.SetMaxResults(PageSize)
.List<ToDoAction>();
var total = StatelessSession.CreateCriteria<ToDoAction>()
.SetProjection(Projections.RowCount())
.UniqueResult<int>();
this.NumberOfPages.Value = total / PageSize + (total % PageSize == 0 ? 0 : 1);
this.Model = new Model
{
Actions = new ObservableCollection<ToDoAction>(actions),
NumberOfPages = NumberOfPages,
CurrentPage = CurrentPage + 1
};
this.CurrentPage.Value = page;
tx.Commit();
}
}
And here is Jose’s:
private void LoadPage(int page)
{
var actions = _toDoActionsDao.RetrieveAll()
.Skip(page * PageSize)
.Take(PageSize).ToList();
var total = _toDoActionsDao.RetrieveAll().Count();
NumberOfPages.Value = total / PageSize
+ (total % PageSize == 0 ? 0 : 1);
Model = new Model
{
Actions = new ObservableCollection<ToDoAction>(actions),
NumberOfPages = NumberOfPages,
CurrentPage = CurrentPage + 1
};
CurrentPage.Value = page;
}
The code is still doing the exact same thing, in other words, we haven’t moved any logic away, and most of the code is dealing with data access details. My approach for testing this is to make use of an in memory database, which result in a single test that make sure that the code works, instead of a series of tests that verifies that each piece work independently and then my single test. I find it much more effective in terms of time & effort.
As for the idea of changing your persistence layer, forget about it. It isn’t going to happen in any real world application without a lot of effort, so you might as well save yourself the effort of working with the lowest common denominator and take full advantage of the framework. Just to note, when I ported the NerdDinner code to NHibernate (an application that make use of a single table), I had to make major changes to the application.
Jose didn’t like this method:
public void OnSave()
{
bool successfulSave;
try
{
using (var tx = Session.BeginTransaction())
{
// this isn't strictly necessary, NHibernate will
// automatically do it for us, but it make things
// more explicit
Session.Update(Model.Action);
tx.Commit();
}
successfulSave = true;
}
catch (StaleObjectStateException)
{
var mergeResult = Presenters.ShowDialog<MergeResult?>("Merge", Model.Action);
successfulSave = mergeResult != null;
ReplaceSessionAfterError();
}
// we call ActionUpdated anyway, either we updated the value ourselves
// or we encountered a concurrency conflict, in which case we _still_
// want other parts of the application to update themselves with the values
// from the db
EventPublisher.Publish(new ActionUpdated
{
Id = Model.Action.Id
}, this);
if (successfulSave)
View.Close();
}
Because:
- ReplaceSessionAfterError, to much responsibility for a presenter.
- Session/Transaction again.
- There is a BUG, the publish mechanism work in sync with the rest of the code. This means… that this windows is not going to close until others have finished handling the event. For the given example, the Edit windows is not going to close until the main window finish to refresh the current page.
- Too much logic for this method = hard to test.
I fully agree that this is a complex method, and Jose’s refactoring for that is an improvement indeed.
[PersistenceConversation(ConversationEndMode = EndMode.End)]
public virtual void OnSave()
{
_toDoActionsDao.Update(Model.Action);
EventPublisher.Enlist(new ActionUpdated
{
Id = Model.Action.Id
}, this);
View.Close();
}
public override void OnException(Exception exception)
{
if(exception is StaleEntityException)
{
Presenters.ShowDialog<MergeResult?>("Merge", Model.Action);
EventPublisher.Enlist(new ActionUpdated
{
Id = Model.Action.Id
}, this);
}
}
Jose has implemented the following changes:
- I’ve defined a new convention, if a public method in the presenter throw an exception, the OnException method will be called. This is done by a castle interceptor, and this is the last line of defense for unhandled exceptions.
- I’m using “StaleEntityException” rather than “StaleObjectStateException”, this is MY exception. This is easily done by a CpBT artifact.
- I’m not calling “EventPublisher.Publish” anymore, this code use EventPublisher.Enlist. Here, I’ve split the “Publish” code in two different methods one for Enlist and other for Raise. The enlisted events will be raised right after the OnSave method is called and thus after the windows is closed.
- Also, notice that here is the conversation per business transaction pattern with all its splendor. The two former methods are conversation participants, with EndMode equals to continue. This means that the NH Session will remain opened. The OnSave method has EndMode equals to End, this means that right after the method finished, CpBT internally will flush the Unit of Work and close it.
This is better than the original implementation, but I think it can be made better still. First, OnException as a generic method is a bad idea. For the simple reason that the exception logic for different method is different, I would probably define a [MethodName]Error(Exception e) convention instead, which would make it easier to separate the error logic for different methods.
Again, I don’t find any usefulness in abstracting the underlying framework, I haven’t seen a single case where it was useful, but I have seen a lot of cases where it was hurting the team & the project.
The idea about splitting the publication and raising is really nice, I agree.
However, there is a problem in the code with regards to session handling in cases of error. There is a pretty good reason why I introduced the ReplaceSessionAfterError method. In general, I want to keep my session alive for the duration of the form, because I get a lot of benefits out of that. But, if the session has run into an error, I need to replace it and all the objects associated with it. Closing the session no matter what is going on is not a good solution, and you can’t really solve the problem in a generic way without calling back to the presenter that generated the error.
+972 52-548-6969
