Repositories and Single Responsibility from the Trenches

In a project I’m involved with we’ve done what I suspect lots and lots of projects do. We’ve used the repository pattern to encapsulate database adds, queries and deletes. We have one such repository per entity type. That worked well for me in earlier projects. But in this case several of the repositories have become somewhat unfocused. None of them are big hairy monsters, but the different public methods really don’t have much in common except the fact that they mainly act on the same entity type. In other words cohesion is low. Let me exemplify (anonymized just as the rest of the code in this post):

public interface IDeviceRepository
    {
      TDeviceType Get<TDeviceType>(long id) where TDeviceType : Device;
      TDeviceType FindByControllerIdentifier<TDeviceType>(string controllerIdentifer)
          where TDeviceType : Device;
    
      IList<TDeviceType> GetAll<TDeviceType>(int offset, int max, 
          DateTimeOffset? createdAfter = null, DateTimeOffset? updatedAfter = null,
          DateTimeOffset? disconnectedAfter = null)
          where TDeviceType : Device;
    
      int Count<TDeviceType>(DateTimeOffset? createdAfter = null,
          DateTimeOffset? updatedAfter = null, DateTimeOffset? disconnectedAfter = null)
        where TDeviceType: Device;
    
      void Add<TDeviceType>(TDeviceType device) where TDeviceType : Device;
      IList<TDeviceType> GetAll<TDeviceType>() where TDeviceType : Device;
      IList<TDeviceType> GetAll<TDeviceType>(long[] deviceIds) where TDeviceType : Device;
      IList<TDeviceType> FindByControllerIdentifiers<TDeviceType>(string[] controllerIdentifiers)
          where TDeviceType : Device;
      IList<NodeDevice> FindNodesDevicesForRootDevice(string controllerIdentifier, 
         bool active = false);
      ICollection<NodeDevice> FindNodeDevicesForRootDevice(long rootDeviceId, bool active = false);
 
      IEnumerable<DeviceLink> GetTopology<T>(long id, bool active) where T : Device;
      DeviceLink GetDeviceLink(string controllerIdentifierA, string controllerIdentifierB);
      DeviceLink GetDeviceLink(long id);
    
      void AddDeviceLink(DeviceLink link);
    
      IList<Route> FindRoutesByDeviceLink(DeviceLink deviceLink);
      void Delete(Device device);
    }

This doesn’t look like the interface of a class that has only one reason to change – i.e. it violates the single responsibility principle (SRP). This is in part because it in fact does not only act on one entity type but on a hierarchy of entity types; device is a base class and the are a handful of concrete types of devices. The obvious solution to that, is to move away from a repository for the base class to a number of repositories for the concrete entities – but that’s where we came from. That’s a route that led to duplicated code, which led to abstracting into a common device repository, so we did not want to go back down that route again.

Taking Single Responsibility Seriously

The solution we’ve come up with is – at it’s core – simply to take SRP seriously: We want classes that do not do a bunch of database related operations around a type of entity. We want classes that does just one such operation. Take the Delete method on the interface above. That seems innocent enough, but it turns out that the implementation is a bit complicated: There are a number of relations that need to be untied, and there is some cascading to take care of before the device can be deleted from the database. As a colleague commented: “It looks like a bad stored proc, written in C#”. We decided to view that delete operation as “one thing”, and decided it justified a class of its own:

 public class DeleteDeviceRequest : NHibernateWriteRequest
    {
      private readonly Device device;
    
      public DeleteDeviceRequest(Device device)
      {
        this.device = device;
      }
    
      protected override void DoExecute()
      {
        var links =
          Session.QueryOver<DeviceLinkReference>().Where(x => x.ControllerIdentifier == device.ControllerIdentifier).
            List();
    
        links.ForEach(x => x.Device = null);
    
        DeleteStatistics();
        new DeleteRoutesWriteRequest(device.Routes).ExecuteWith(DataStore);
        DeleteFromGroup();
        DeleteFromOrder();
        DeleteProfiles();
        DataStore.Delete(device);
      }
    
     // more private methods...

The only thing public here is the constructor. The constructor takes all arguments for the delete, so once the object is constructed it completely encapsulates the delete. And – importantly – it is incapable of doing anything else than that particular delete.
How is the delete executed? -Through an execute method on the base class NHibernateWriteRequest.

This swaps the repository pattern for the command pattern plus the template method pattern. But more importantly this swaps a small collection of ever growing repositories for a large collection of small never growing query classes (which, btw, also plays better into the open/closed principle). We’ve only just started down this path, but I think we’ll be happy about it.

There are a few twists I’ve left out, particularly around making these request objects easy to get at for client code, while maintaining encapsulation and testability. I might revisit those in a future post. 

First I’ll re-iterate how one of these request classes look (NB. compared to the code from the last post the base class has been renamed):

 public class DeleteDeviceWriteRequest : DataStoreWriteRequest
    {
      private readonly Device device;
    
      public DeleteDeviceWriteRequest(Device device)
      {
        this.device = device;
      }
    
      protected override void DoExecute()
      {
        // NHibernate trickery cut out for brewity
        Session.Delete(device);
      }
    }

And below I show how to use, test and mock this class.

Usage

Usage of these requests is really simple: You just new one up, and ask your local data store object to execute it:

 var deleteDeviceRequest = new DeleteDeviceWriteRequest(meter);
 dataStore.Execute(deleteDeviceRequest);

Say, what? I’m newing up an object that uses NHibernate directly. Gasp. That’s a hard coupling to the ORM and to the database, isn’t it? Well, kind of. But that is where the data store object comes into play: The request can only be executed through that object, because the request is only public method is its constructor and because it’s base class ‘DataStoreWriteRequest’ has no public methods. The interface for that data store is:

public interface IDataStore
    {
      void Execute(DataStoreWriteRequest req);
      T Execute<T>(DataStoreReadRequest<T> req);
      T Get<T>(long id) where T : class;
      T Get<T>(Expression<Func<T,bool>> where) where T : class;
      void Add<T>(T entity) where T : class;
      void Delete<T>(T entity) where T : class;
      int Count<T>(Expression<Func<T, bool>> where = null) where T : class;
    }

That could be implemented towards any database/ORM. I our case it’s implemented against NHibernate, and is pretty standard except maybe for the two execute methods – but then again they turn out to be really straightforward as well:

public void Execute(DataStoreWriteRequest req)
   {
     WithTryCatch(() => req.ExecuteWith(Session));
   }
  
   public T Execute>T>(DataStoreReadRequest>T> req)
   {
     return WithTryCatch(() => req.ExecuteWith(Session));
   }
   
   private void WithTryCatch(Action operation)
   {
     WithTryCatch(() => { operation(); return 0; });
   }
   
   private TResult WithTryCatch>TResult>(Func>TResult> operation)
   {
     try
     {
       return operation();
     }
     catch (Exception)
     {
       Dispose(); // ISession must be disposed
       throw;
     }
  }

Notice the calls to ExecuteWith? Those are calls to internal methods on the abstract DataStoreReadRequest and DataStoreWriteRequest classes. In fact those internal methods are the reason that DataStoreReadRequest and DataStoreWriteRequest exists. Using a template method declared internal they provide inheritors – the concrete data base requests – a way to get executed, while hiding everything but the contrustors from client code. Only our NHibernate implementation of IDataStore ever calls the ExecuteWith methods. All the code outside the our data access assembly can not even see those methods. As it turns out this is really simple code as well:

public abstract class DataStoreWriteRequest
      {
        protected ISession Session { get; private set; }
     
         internal void ExecuteWith(ISession seesion)
        {
          Session = seesion;
          DoExecute();
        }
    
        protected abstract void DoExecute();
      }

To sum up; the client code just news the requests it needs, and then hands them off to the data store object. Simple. Requests only expose constructors to the client code, nothing else. Simple.

Testing the Requests

Testing the requests individually is as simple as using them. This is no surprise since tests – as we know – are just the first clients. The tests do whatever setup of the database they need, then new up the request and the data store, asks the data store to execute the request, and then asserts. Simple. Just like the client production code.

In fact one of the big wins with this design over our old repositories is that the tests become a whole lot simpler: Although you can split up tests classes in many ways, the reality for us (as I suspect it is for many others too) is that we tend to have one test class per production class. Sometimes two, but almost never three or more. Since the repository classes grew and grew so did the corresponding test classes resulting in some quite hairy setup code. With the new design each test class tests just one very specific request leading to much, much more cohesive test code.

To illustrate here is the first simple of test for the above DeleteDeviceRequest – note that the UnitOfWork objects in this test implement IDataStore:

[Test]
       public void ShouldDeleteDeviceWithNoRelations()
       {
         var device = new Device();
         using (var arrangeUnitOfWork = CreateUnitOfWork())
         {
           arrangeUnitOfWork.Add(device);
         }
    
         using (var actUnitOfWork = CreateUnitOfWork())
         {
           var sut = new DeleteDeviceWriteRequest(device);
           actUnitOfWork.Execute(sut);
         }
    
         using (var assertUnitOfWork = CreateUnitOfWork())
         {
           Assert.That(assertUnitOfWork.Get<Device>(device.Id), Is.Null);
         }
       }

Mocking the Request

The other part of testing is testing the code that uses these requests; testing the client code. For those tests we don’t want the request to be executed, since we don’t want to get slowed down by those tests hitting the database. No, we want to mock the requests out completely. But there is a catch: The code under test like the code in the first snippet in the Usage section above new’s up the request. That’s a hard compile time coupling to the concrete request class. There is no seam allowing us to swap the implementation. What we’re doing about this is that we’re sidestepping the problem, by mocking the data store object instead. That allows us redefine what executing the request means: Our mock data store never executes any of the requests it’s asked to execute, it just records that it was asked to execute a certain request, and in the case of read requests returns whatever object we set it up to return. So the data store is our seam. The data store is never newed up directly in production code, it’s always injected through constructors. Either by the IoC/DI container or by tests as here:

[Test]
   public void DeleteDeviceRestCallExectuesDeleteOnDevice()
   {
     var dataStore = mock.StrictMock<IDataStore>();
     var sut = new RestApi(dataStore);
   
     var device = new Device { Id = 123 };
   
     Expect.Call(unitOfWork.Get<Device>(device.Id)).Return(device);
     Expect.Call(() => 
       unitOfWork.Execute(
       Arg<DeleteMeterWriteRequest>
       .Is.Equal(new DeleteMeterWriteRequest(device))));
   
     mock.ReplayAll();
   
     sut.DeleteMeter(device.Id.ToString());
   
     mock.VerifyAll();    
   }

(the above code uses Rhino.Mocks to mock out the data store, but that’s could have been done quite simply by hand as well or by any other mocking library)

That’s it. :-)

Reference: Repositories and Single Responsibility from the Trenches, Repositories and Single Responsibility from the Trenches – Part II from our NCG partner Christian Horsdal at the Christian Horsda’s blog blog.