вторник, 24 сентября 2013 г.

IQueryable is root of many problems

It's often suggested to avoid unnecessary abstractions in your projects. Some respectful community members like to repeat it over and over again. But how to distinguish between required and unnecessary abstraction? What values can abstraction bring in?

Let's take a look at sample code which uses NHibernate's session to access data.

 internal sealed class TaskCoordinator
 {
  private readonly ISession _session;
 
  public TaskCoordinator(ISession session)
  {
   _session = session;
  }
 
  public void DoSomething()
  {
   /* ... */
 
   var tasks = GetRunningTasks(1);
 
   /* ... */
 
   foreach (var task in tasks)
   {
    var previousTask = GetPreviousTask(task);
 
    /* ... */
   }
  }
 
  private Task[] GetRunningTasks(int workflowId)
  {
   var utcNow = DateTime.UtcNow;
   return _session.Query<Task>()
    .Where(t => t.IsActive)
    .Where(t => t.WorkflowId == workflowId)
    .Where(t => t.StartsAt <= utcNow)
    .Where(t => t.ExpiresAt > utcNow)
    .ToArray();
  }
 
  private Task GetPreviousTask(Task task)
  {
   return _session.Query<Task>()
    .Where(t => t.WorkflowId == task.WorkflowId)
    .Where(t => t.StartsAt < task.StartsAt)
    .Where(t => t.ExpiresAt <= task.StartsAt)
    .FirstOrDefault();
  }
 }


Problem is: should we use session directly or should be hide it under another level of abstraction.
But before making any decision let me ask you a few questions:

  1. Except type safety, what is the difference between these LINQ-queries and SQL statements used directly in code?
  2. Without studying whole codebase, can you tell me what are use cases and what indices should be created in database to cover these use cases?
  3. Can you estimate L2 cache hit ratio for your queries?
  4. Can you easily (easily means isolated, without building two-page test setup) test buggy method GetPreviousTask?
  5. Can you write unit (means without involving database) tests for TaskCoordinator?
These questions and your answers help you make informed decision and choose between previous implementation and introducing another level of abstraction:


 public interface ITaskRepository
 {
  Task[] GetRunningTasks(int workflowId);
  
  Task GetPreviousTask(Task task);
 }
 
 internal sealed class TaskCoordinator
 {
  private readonly ITaskRepository _repository;
 
  public TaskCoordinator(ITaskRepository repository)
  {
   _repository = repository;
  }
 
  public void DoSomething()
  {
   /* ... */
 
   var tasks = _repository.GetRunningTasks(1);
 
   /* ... */
 
   foreach (var task in tasks)
   {
    var previousTask = _repository.GetPreviousTask(task);
 
    /* ... */
   }
  }
 }

Actually title is provoking, it's not the IQueryable problem, it's a problem of any wide unbounded contract.

5 комментариев:

  1. How will ITaskRepository help to answer question #3? This question suggests to hide the session under another abstraction, don't it?

    ОтветитьУдалить
  2. Let's imagine in original code DoSomething has cyclomatic complexity more than 1 and GetPreviousTask is called only under some condition. To test this method you should setup you test environment so condition will be met and GetPreviosTask will be called. It can be non-trivial and not an easy (which is part of the question) task. With repository your test should create 3 consequent tasks in database and call repository's GetPreviousTask three time for each of created tasks, validating output. It is really trivial, trust me :)

    ОтветитьУдалить
    Ответы
    1. I see now. I thought you meant it should be isolated from database as well. Thanks.

      Удалить
  3. Depends how much code you are ready to write for testability. Actually both samples smells bad. First because of ISession in ctor, second because of theoretical performance gain.
    Think more generic:
    public interface ITaskRepository {
    IQueryable QueryTasks();
    Task GetPreviousTask(Task task);
    }
    No performance gain, your unit test looks like
    var stub = Stub().Setup(x=>x.QueryTasks())
    .Return(new List{..}.ToQueryable())
    var component = new TaskCoordinator(stub);

    ОтветитьУдалить
  4. What's wrong to have ISession in ctor? And with your ITaskRepository, could you easily answer questions 2 and 3? What I mean is - with IQueryable you don't understand how your storage is used and how to optimize it.

    ОтветитьУдалить

Wider Two Column Modification courtesy of The Blogger Guide