José F. Romaniello

Las aventuras y desventuras de un codificador.

I started to write a review about Ayende Rahien’s sample application Effectus, but I soon realized that what  I do better is to write code. So, this is an unconventional review, because I will focus on some parts of the code, and then show you the alternative.

You can read more about Effectus in Ayende Rahien’s blog.

Straight to the point

In this post I will focus only in the presenters of Effectus. Because this is the first thing that I don’t like.

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();
    }
}

Ah? StatelessSession?, BeginTransaction?, CreateCriteria?. 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.

So, pay attention to Conversation per Business Transaction. Fabio Maulo did a great job with this and I really like it.

Main\Presenter

Ayende’s code

namespace Effectus.Features.Main
{
    using System.Collections.ObjectModel;
    using System.ComponentModel;
    using Effectus.Model;
    using Events;
    using Infrastructure;
    using NHibernate.Criterion;
 
    public class Presenter : AbstractPresenter<Model, View>
    {
        const int PageSize = 3;
 
        public Observable<int> NumberOfPages { get; set; }
 
        public Observable<int> CurrentPage { get; set; }
 
        public Presenter()
        {
            NumberOfPages = new Observable<int>();
            CurrentPage = new Observable<int>();
 
            EventPublisher.Register<ActionUpdated>(RefreshCurrentPage);
        }
 
        private void RefreshCurrentPage(ActionUpdated _)
        {
            LoadPage(CurrentPage);
        }
 
        public Fact CanMovePrev
        {
            get { return new Fact(CurrentPage, 
                        () => CurrentPage > 0); }
        }
 
        public Fact CanMoveNext
        {
            get { return new Fact(CurrentPage, 
                        () => CurrentPage + 1 < NumberOfPages); }
        }
 
        public void OnCreateNew()
        {
            Presenters.Show("CreateNew");
        }
 
        public void OnActionsChoosen(ToDoAction action)
        {
            Presenters.Show("Edit", action.Id);
        }
 
        public void OnLoaded()
        {
            LoadPage(0);
        }
 
        public void OnMoveNext()
        {
            LoadPage(CurrentPage + 1);
        }
 
        public void OnMovePrev()
        {
            LoadPage(CurrentPage - 1);
        }
 
        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();
            }
        }
    }
}

My code

using System.Linq;
using Effectus.DataAccess;
using uNhAddIns.Adapters;

namespace Effectus.Features.Main
{
    using System.Collections.ObjectModel;
    using Effectus.Model;
    using Events;
    using Infrastructure;

    [PersistenceConversational(
        DefaultEndMode = EndMode.End, 
        MethodsIncludeMode = MethodsIncludeMode.Explicit)]
    public class Presenter : AbstractPresenter<Model, View>
    {
        private readonly IReadOnlyDao<ToDoAction> _toDoActionsDao;
        const int PageSize = 3;

        public Observable<int> NumberOfPages { get; set; }

        public Observable<int> CurrentPage { get; set; }

        public Presenter(IReadOnlyDao<ToDoAction> toDoActionsDao)
        {
            _toDoActionsDao = toDoActionsDao;
            NumberOfPages = new Observable<int>();
            CurrentPage = new Observable<int>();

            EventPublisher.Register<ActionUpdated>(RefreshCurrentPage);
        }

        [PersistenceConversation]
        public virtual void RefreshCurrentPage(ActionUpdated _)
        {
            LoadPage(CurrentPage);
        }

        public Fact CanMovePrev
        {
            get { return new Fact(CurrentPage, 
                            () => CurrentPage > 0); }
        }

        public Fact CanMoveNext
        {
            get { return new Fact(CurrentPage, 
                            () => CurrentPage + 1 < NumberOfPages); }
        }

        public void OnCreateNew()
        {
            Presenters.Show("CreateNew");
        }

        public void OnActionsChoosen(ToDoAction action)
        {
            Presenters.Show("Edit", action.Id);
        }

        [PersistenceConversation]
        public virtual void OnLoaded()
        {
            LoadPage(0);
        }
        
        [PersistenceConversation]
        public virtual void OnMoveNext()
        {
            LoadPage(CurrentPage + 1);
        }

        [PersistenceConversation]
        public virtual void OnMovePrev()
        {
            LoadPage(CurrentPage - 1);
        }

        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;
        }
    }
}

There are two things that you will notice:

  • A dependency with IReadonlyDao<ToDoAction>. I will show you later the code for daos.
  • Conversation per business transactions attributes. These attributes are from a separated assembly unrelated to nhibernate. This means that we could implement this for EntityFramework or other ORM and the presenter is not coupled to the persistence framework. This will pass the “acid” test, thanks Angel “Java” Lopez.

Ah, there is a last thing about that code. There is a bug with the current page number.

AbstractPresenter

Ayende’s Code

namespace Effectus.Infrastructure
{
    using System;
    using System.Windows;
    using NHibernate;

    public abstract class AbstractPresenter<TModel, TView> : IDisposable, IPresenter
        where TView : Window, new()
    {
        private TModel model;
        private ISessionFactory sessionFactory;
        private ISession session;
        private IStatelessSession statelessSession;

        protected AbstractPresenter()
        {
            View = new TView();
            View.Closed += (sender, args) => Dispose();
        }

        DependencyObject IPresenter.View { get { return View; } }

        public object Result { get; protected set; }

        protected TView View { get; set; }

        protected ISessionFactory SessionFactory
        {
            get { return sessionFactory; }
        }

        protected ISession Session
        {
            get
            {
                if (session == null)
                    session = sessionFactory.OpenSession();
                return session;
            }
        }

        protected IStatelessSession StatelessSession
        {
            get
            {
                if (statelessSession == null)
                    statelessSession = sessionFactory.OpenStatelessSession();
                return statelessSession;
            }
        }

        protected TModel Model
        {
            get { return model; }
            set
            {
                model = value;
                View.DataContext = model;
            }
        }

        protected void ReplaceSessionAfterError()
        {
            if(session!=null)
            {
                session.Dispose();
                session = sessionFactory.OpenSession();
                ReplaceEntitiesLoadedByFaultedSession();
            }
            if(statelessSession!=null)
            {
                statelessSession.Dispose();
                statelessSession = sessionFactory.OpenStatelessSession();
            }
        }

        protected virtual void ReplaceEntitiesLoadedByFaultedSession()
        {
            throw new InvalidOperationException(
                @"ReplaceSessionAfterError was called, but the presenter does not override ReplaceEntitiesLoadedByFaultedSession!
You must override ReplaceEntitiesLoadedByFaultedSession to call ReplaceSessionAfterError.");
        }


        public void SetSessionFactory(ISessionFactory theSessionFactory)
        {
            sessionFactory = theSessionFactory;
        }

        public void Show()
        {
            View.Show();
        }

        public void ShowDialog()
        {
            View.ShowDialog();
        }

        public event Action Disposed = delegate { };

        public virtual void Dispose()
        {
            if(session!=null)
                session.Dispose();
            if (statelessSession != null)
                statelessSession.Dispose();
            View.Close();
            Disposed();
        }
    }
}

My code

namespace Effectus.Infrastructure
{
    using System;
    using System.Windows;
    using NHibernate;

    public abstract class AbstractPresenter<TModel, TView> : IDisposable, IPresenter
        where TView : Window, new()
    {
        private TModel model;

        protected AbstractPresenter()
        {
            View = new TView();
            View.Closed += (sender, args) => Dispose();
        }

        DependencyObject IPresenter.View { get { return View; } }

        public object Result { get; protected set; }

        protected TView View { get; set; }

        protected TModel Model
        {
            get { return model; }
            set
            {
                model = value;
                View.DataContext = model;
            }
        }

        public void Show()
        {
            View.Show();
        }

        public void ShowDialog()
        {
            View.ShowDialog();
        }

        public event Action Disposed = delegate { };

        public virtual void Dispose()
        {
            View.Close();
            Disposed();
        }

        public virtual void OnException(Exception exception)
        {
            //TODO You should improve this.
            MessageBox.Show(exception.Message);
        }
    }
}

Again, I’ve deleted all NHibernate code.

Edit\Presenter

Ayende’s code

namespace Effectus.Features.Edit
{
    using System;
    using System.Windows;
    using Effectus.Model;
    using Events;
    using Infrastructure;
    using Merge;
    using NHibernate;

    public class Presenter : AbstractPresenter<Model, View>
    {
        public Presenter()
        {
            EventPublisher.Register<ActionUpdated>(RefreshAction);
        }

        private void RefreshAction(ActionUpdated actionUpdated)
        {
            if (actionUpdated.Id != Model.Action.Id)
                return;
            Session.Refresh(Model.Action);
        }

        public void Initialize(long id)
        {
            ToDoAction action;
            using (var tx = Session.BeginTransaction())
            {
                action = Session.Get<ToDoAction>(id);
                tx.Commit();
            }

            if (action == null)
                throw new InvalidOperationException("Action " + id + " does not exists");

            this.Model = new Model
            {
                Action = action
            };
        }
        [PersistenceConversation(ConversationEndMode = EndMode.Abort)]
        public void OnCancel()
        {
            View.Close();
        }

        public void OnCreateConcurrencyConflict()
        {
            using (var session = SessionFactory.OpenSession())
            using (var tx = session.BeginTransaction())
            {
                var anotherActionInstance = session.Get<ToDoAction>(Model.Action.Id);
                anotherActionInstance.Title = anotherActionInstance.Title + " - ";
                tx.Commit();
            }
            MessageBox.Show("Concurrency conflict created");
        }

        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();
        }

        protected override void ReplaceEntitiesLoadedByFaultedSession()
        {
            Initialize(Model.Action.Id);
        }
    }
}

There are several things with the OnSave method that I don’t like:

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

My code

using Effectus.DataAccess;
using Effectus.Features.Merge;
using Effectus.Infrastructure.Bootstrapping;
using Effectus.Infrastructure.ConversationsArtifacts;
using uNhAddIns.Adapters;
using uNhAddIns.SessionEasier.Conversations;

namespace Effectus.Features.Edit
{
    using System;
    using System.Windows;
    using Effectus.Model;
    using Events;
    using Infrastructure;
    using NHibernate;

    [PersistenceConversational(
            DefaultEndMode = EndMode.Continue, 
            UseConversationCreationInterceptorConvention = true)]
    public class Presenter : AbstractPresenter<Model, View>
    {
        private readonly IDao<ToDoAction> _toDoActionsDao;

        public Presenter(IDao<ToDoAction> toDoActionsDao)
        {
            _toDoActionsDao = toDoActionsDao;
            EventPublisher.Register<ActionUpdated>(RefreshAction);
        }
        
        [PersistenceConversation]
        public virtual void RefreshAction(ActionUpdated actionUpdated)
        {
            if (actionUpdated.Id != Model.Action.Id)
                return;

            _toDoActionsDao.Refresh(Model.Action);
        }

        [PersistenceConversation]
        public virtual void Initialize(long id)
        {
            var action = _toDoActionsDao.GetById(id);

            if (action == null)
                throw new InvalidOperationException("Action " + id + " does not exists");

            Model = new Model
            {
                Action = action
            };
        }

        public void OnCancel()
        {
            View.Close();
        }

        public void OnCreateConcurrencyConflict()
        {
            //NOTE: this portion of code is for generating a conflict.
            //don't use sessionfactory in presenters.

            var sessionFactory = BootStrapper.Container.Resolve<ISessionFactory>();

            using (var session = sessionFactory.OpenSession())
            using (var tx = session.BeginTransaction())
            {
                var anotherActionInstance = session.Get<ToDoAction>(Model.Action.Id);
                anotherActionInstance.Title = anotherActionInstance.Title + " - ";
                tx.Commit();
            }
            MessageBox.Show("Concurrency conflict created");
        }

        [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);

            }
        }
    }

}

Pay atention:

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

 

The code needed for exception interception:

public class RecoverableExceptionIntercepter : IInterceptor
{
    public void Intercept(IInvocation invocation)
    {
        if(!typeof(ISupportExceptionRecovery).IsAssignableFrom(invocation.TargetType))
        {
            throw new NotSupportedException(string.Format("{0} should implement {1}", 
                invocation.TargetType.Name, 
                typeof(ISupportExceptionRecovery).Name));
        }

        try
        {
            invocation.Proceed();
        }catch(Exception ex)
        {
            var recoverablePresenter = (ISupportExceptionRecovery) invocation.InvocationTarget;
            recoverablePresenter.OnException(ex);
        }

    }
}

The code needed for auto-flushing published events:

public class AutoFlushEventPublisherEvents : IInterceptor
{
    public void Intercept(IInvocation invocation)
    {
        invocation.Proceed();
        EventPublisher.FlushEvents((IPresenter) invocation.InvocationTarget);
    }
}

The code for daos

Interfaces:

public interface IReadOnlyDao<T>
    where T : EntityBase 
{
    T GetById(object id);
    T GetProxy(object id);
    IEnumerable<T> Retrieve(Expression<Func<T, bool>> predicate);

    int Count(Expression<Func<T, bool>> predicate);

    /// <summary>
    /// Retrieve all entities.
    /// </summary>
    /// <returns></returns>
    IEnumerable<T> RetrieveAll();
}

public interface IDao<T> : IReadOnlyDao<T>
    where T : EntityBase 
{
    void Save(T entity);
    void Update(T entity);
    void Delete(T entity);
    
    /// <summary>
    /// Re-read the state of the given instance from the underlying database.
    /// </summary>
    /// <param name="entity"></param>
    void Refresh(T entity);

    /// <summary>
    /// Merge the instance in to the persisted instance of the current unit of work.
    /// </summary>
    /// <param name="entity"></param>
    void Merge(T entity);

}

These interfaces were taken from Fabio Maulo’s post “Repository or DAO”?: Dao.

Implementation:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using Effectus.Model;
using NHibernate;
using NHibernate.Criterion;
using NHibernate.Linq;

namespace Effectus.DataAccess
{
    public class Dao<T> : IDao<T>
        where T : EntityBase
    {
        private readonly ISessionFactory _sessionFactory;

        public Dao(ISessionFactory sessionFactory)
        {
            _sessionFactory = sessionFactory;
        }

        private IQueryable<T> CurrentLinq
        {
            get { return ContextSession.Linq<T>(); }
        }

        private ISession ContextSession
        {
            get { return _sessionFactory.GetCurrentSession(); }
        }

        #region IDao<T> Members

        public void Save(T entity)
        {
            ContextSession.Save(entity);
        }

        public void Update(T entity)
        {
            ContextSession.Update(entity);
        }

        public void Delete(T entity)
        {
            ContextSession.Delete(entity);
        }

        public void Refresh(T entity)
        {
            ContextSession.Refresh(entity);
        }

        public void Merge(T entity)
        {
            ContextSession.Merge(entity);
        }

        public T GetById(object id)
        {
            return ContextSession.Get<T>(id);
        }

        public T GetProxy(object id)
        {
            return ContextSession.Load<T>(id);
        }

        public IEnumerable<T> Retrieve(
                    Expression<Func<T, bool>> predicate)
        {
            return CurrentLinq.Where(predicate);
        }

        public IEnumerable<T> RetrieveAll()
        {
            return CurrentLinq;
        }

        public int Count(Expression<Func<T, bool>> predicate)
        {
            return CurrentLinq.Count(predicate);
        }

        #endregion
    }

    
}

Download

Here is the full code. Maybe someone help me to “fork” github version, this will be very nice to make code diff.

The end

There is nothing more, I like very much the result of this review and I will be waiting your comments (please let me a comment). I hope you like it!

| More

16 comentarios:

Did you noice that it's a _sample_ application?

José F. Romaniello dijo...

Yes, of course. My review is also a "review of a sample". I have others fees
for other kind of reviews.
But Oren's is ok about me reviewing: http://digg.com/u1IRrX
Sorry, but I don't get your comment.

You seem to be willing to point out several wrong things, without considering that it's not production code and hence there's nothing wrong with them.

José F. Romaniello dijo...

No, I'm willing to point out an alternative way to handle the wrong things.
I'm not "complaining" to Oren, I really like his example and I really enjoy
his msdn article.
I'm just fixing some things and showing my way. I think my readers and in
general the people will benefit from this review.
I've seem less constructive reviews of examples application where the
reviewer only complains about the example and the author. (guess who write
those reviews?)
Why I can't point out wrong things?

While like you, I don't like the gluing of NHibernate to the presenters. It's not such a bad thing. If you think about it, your not going to remove a data layer like NHibernate, when the main reason for being able to swap DAL's is to change storage engines, which you can do with NHibernate without removing NHibernate.

However, it does feel icky, so I DO agree with you :)

José F. Romaniello dijo...

Thank you for your comment.
Yes these are different concerns. I don't test presenters, mapping, data
access and so on in the same fixture.

Oh, very nice. I completely agree on abstracting the presenters from the persistance. BTW, it doesn't care if this is NASA production code or someone's pet project, always there's room for improvement :)
Thanks José.

José F. Romaniello dijo...

<span style="color: #808080;">Thank you, for your comment.</span>
I've an special fee for Nasa production code today  :-D  

So... is it a _sample_ application... with several wrong things... did you ever noticed sample applications are done just to point out *how* to make something right? I mean, did you remember Oxite right?

Excellent!. I always knew those Lecops in the bottom of my drawer will be useful someday :P

I like the idea of abstracting away the persistence layer from the presenter.

However, we must not forget that this is a small application that serves as an example to an article that is all about using NHibernate in a Windows Client App. And now you are abstracting away that what this article is all about.
Furthermore, we must not forget that for small applications and especially apps that serve as a demo, it is not at all bad to follow the approach Ayende chooses. The reasons to abstract away the persistence layer is for testability, which is hardly necessary for small applications like the one we are talking about. Another reason would be to easily replace the persistence-layer. But, imho, there is no good reason why I would replace NHibernate as persistance layer, because NHibernate is very feature rich, well-tested, and can handle all commonly used databases on the market.

Do not get me wrong...I agree with you also when it concerns bigger applications that are to be used in production. But in this context, it is not necesarry.

One small note on the code: there is probably an error in the Enlist<t> static method of the EventPublisher class. In the else-part you do an eventsToPublish.Add(eventsToPublish), which should be eventsToPublish.Add(eventToPublish)

Good article anyway!

José F. Romaniello dijo...

Thank you very much for your comment.
I know this is an example application and I like very much. But why it can't
be _better_?
So, for small applications you build a eventpublisher mechanism, and you use
MVP or MVVM pattern? even if you are not going to test it?
So following your idea: we can spread this code all over the code behind,
because it is a sample and is small.
Don't get me wrong, if a sample is not meant to be used, Why is usefull for?

Nice post.  I'm learning NHibernate at the moment and wondered how you might use the DAO approach and control things such as lazy or eager loads of entities ?  I like the seperation of concerns (of course) but it would seem that I lose some control over NHibernate.  A simple example might be to have blog and comment entities.  In some scenarios I want the comments to be loaded and in others I don't.

Darren

José F. Romaniello dijo...

<span>Well, good question. My IDao has method Retrieve(Expression<func>>) that use linq, to retrieve an iqueryable. But also I've a method Retrieve(Expression<func>>, params string[]) </func></func></span>

The second parameter is the "expanded" or eager loading those properties. Usually I only expand once.
This is not sufficient in other cases were I prefer to have an specialized hql query. Then, I can wrap in a "IBestProductByMonth" something like that an implement in the DAL. This object depends on sessionfactory to open an stateless session or a normal session.

Hi there,

Can you re-post somewhere the source code? The link provided in the article is broken.

Thank you.

José F. Romaniello dijo...

<span>Sorry, and thank you for reporting. The link now works!</span>

Publicar un comentario en la entrada