Geeks With Blogs

News


Dylan Smith ALM / Architecture / TFS

So the other day at work we were doing an architecture/design review on a project to share knowledge gained with other project teams.  After making the code available one of the other architects came to me with a specific piece of the code in hand asking us to justify why it seemed so complex (in this case the comment was mainly because the code used lambda's generously and he had limited exposure to them previously).  Here is the code in question:

Original Approach

public void Save(IJob job)
{
    this.AutoCommit(s => s.SaveOrUpdate(job));
}

public static void AutoCommit(this IDisplacementStorage storage, Action<IStorageSession> action)
{
    storage.AutoSession(s =>
                            {
                                using (var tx = s.BeginTransaction())
                                {
                                    try
                                    {
                                        action(s);

                                        tx.Commit();
                                    }
                                    catch
                                    {
                                        tx.Rollback();

                                        throw;
                                    }
                                }
                            });
}

public static void AutoSession(this IDisplacementStorage storage, Action<IStorageSession> action)
{
    using (var session = storage.OpenSession())
    {
        action(session);
    }
}
 
I wanted to compare this implementation to some more simplistic implementations.

Option 1

public void Save(IJob job)
{
    using (var session = this.OpenSession())
    {
        using (var tx = session.BeginTransaction())
        {

            try
            {
                session.SaveOrUpdate(job);
                tx.Commit();
            }
            catch
            {
                tx.Rollback();
                throw;
            }
        }
    }
}

 

Aside from clearly breaking the Single Responsibility Principle the Option 1 approach is less code (8 lines in one method vs 11 lines in 3 methods) and also easier to understand/read.  However the SRP problems become more evident once you start to flesh out the storage layer and need to implement more Save methods.  Lets say we needed a Save(IZone), Save(IRoom), Save(IDiffuser), etc.  We could just copy/paste the code and change the argument type, but that would quickly result in much more code.  The Original Approach only requires 1 extra method containing one line of code for each additional Save method.  The Option 1 approach would require an extra method containing 8 lines of code for each additional save method.  It also results in lots of code duplication between Save method, so if you wanted to change the way you create sessions/transactions, or add logging, or change the error handling, or anything like that you have to make changes in many places as opposed to just one in the Original Approach.  An obvious answer to some of these concerns is to refactor out the common code.  See Option 2 below.

 

Option 2

public void Save(IJob job)
{
    SaveOrUpdate(job);
}

public void Save(IZone zone)
{
    SaveOrUpdate(zone);
}

public void SaveOrUpdate(object target)
{
    using (var session = this.OpenSession())
    {
        using (var tx = session.BeginTransaction())
        {

            try
            {
                session.SaveOrUpdate(target);
                tx.Commit();
            }
            catch
            {
                tx.Rollback();
                throw;
            }
        }
    }
}
 

The SRP violations are getting better now, the Save method clearly only has one responsibility now, although the SaveOrUpdate is still taking on multiple responsibilities (creating/disposing session, creating/committing/rolling-back transaction, saving the object).  The Option 2 approach now has 9 lines of code spread across 2 methods – still better than the Original Approach in this respect.  Adding additional Save methods now simply requires adding a new method with a single line of code, which is the same as in the Original Approach.  However, what about deletes?  To handle that we could create a similar method as the SaveOrUpdate but to handle deletes; however, that would be another 8 lines of code and an additional method for deletes.  What about more complex scenarios, for example lets say it was a financial application and you had a TransferFunds method that needed up to update 2 account balances within a transaction.  In that case or other cases that are more than just a simple save/delete on a single object we’ll need to essentially copy/paste the code to create/dispose the session + transaction each time along with the appropriate try/catch block.  A nicer way to handle arbitrary scenarios like this is instead of hardcoding the action to be taken we allow the caller to pass in an Action that contains the scenario-specific data-access code.  This might look something like Option 3 below.

 

Option 3

public void Save(IJob job)
{
    ExecuteInTransaction(s => s.SaveOrUpdate(job));
}

public void Delete(IJob job)
{
    ExecuteInTransaction(s => s.Delete(job));
}

public void TransferFunds(IAccount fromAccount, IAccount toAccount)
{
    ExecuteInTransaction(s => 
                              {
                                  s.SaveOrUpdate(fromAccount); 
                                  s.SaveOrUpdate(toAccount);
                              });
}

public void ExecuteInTransaction(Action<IStorageSession> action)
{
    using (var session = this.OpenSession())
    {
        using (var tx = session.BeginTransaction())
        {

            try
            {
                action(session);
                tx.Commit();
            }
            catch
            {
                tx.Rollback();
                throw;
            }
        }
    }
}

 

The lines of code required for just the save is the same as in Option 2 (9 lines of code spread across 2 methods) but now we have much more flexibility to do other data-access scenarios such as delete or more complex use cases with minimal code required.  The ExecuteInTransaction class arguably still violates the SRP because it is responsible for handling the session creation/disposition as well as handling the transaction.  We could break this out to satisfy SRP; but a more reasonable justification would be that you might want to be able to execute data-access code outside of a transaction, or you might want to directly manage the transaction object rather than the simple transaction handling baked into the ExecuteInTransaction method.  We could do this by refactoring out the session handling code into it’s own method.  We can follow the same pattern we did with using the Action in the ExecuteInTransaction method to allow the caller to pass in arbitrary actions to be executed in the session.

 

Option 4

public void Save(IJob job)
{
    ExecuteInTransaction(s => s.SaveOrUpdate(job));
}

public void Delete(IJob job)
{
    ExecuteInTransaction(s => s.Delete(job));
}

public void TransferFunds(IAccount fromAccount, IAccount toAccount)
{
    ExecuteInTransaction(s => 
                              {
                                  s.SaveOrUpdate(fromAccount); 
                                  s.SaveOrUpdate(toAccount);
                              });
}

public void ExecuteInTransaction(Action<IStorageSession> action)
{
    ExecuteInSession(s =>
                        {
                            using (var tx = s.BeginTransaction())
                            {
                                try
                                {
                                    action(s);
                                    tx.Commit();
                                }
                                catch
                                {
                                    tx.Rollback();
                                    throw;
                                }
                            }
                        });
}

public void ExecuteInSession(Action<IStorageSession> action)
{
    using (var session = this.OpenSession())
    {
        action(session);
    }
}
 

If we rename ExecuteInTransaction -> AutoCommit and rename ExecuteInSession -> AutoSession we now have the exact code we started off with in the Original Approach (except that the Original Approach implemented them as extension methods whereas Option 4 is written as instance methods directly on IDisplacementStorage).

Posted on Wednesday, September 30, 2009 6:28 PM | Back to top


Comments on this post: Cutting Through Complexity – Lambdas

No comments posted yet.
Your comment:
 (will show your gravatar)


Copyright © Dylan Smith | Powered by: GeeksWithBlogs.net