KISS my... unit of work

Question

I've seen many, many UoW+Repository implementations. Whenever one was built on top of Entity Framework, I'd cringe at the added complexity.

Sure the complexity buys you (sometimes) full decoupling from Entity Framework, which in theory would possibly allow swapping EF for, say, NHibernate, or even "downgrade" to a lower-level ADO.NET solution.

But most people implement an IUnitOfWork interface for the sole purpose of enabling easier testability - I'm one of those. I'm not buying the "dude I can swap my ORM without touching the rest of my app" argument, simply because it's more often than not, a theoretical advantage, if not a plain lie.

So I like to keep it stupid simple (KISS), and only do the minimum needed to enable mocking of my DbContext class - here's a real-world IUnitOfWork interface I've written in a recent project:

IUnitOfWork interface

public interface IUnitOfWork
{
    int SaveChanges();
    IDbSet<TEntity> Repository<TEntity>() where TEntity : class;
}

Entity Framework's DbContext is a unit of work, and IDbSet<TEntity> is a repository. So instead of going all out and writing a generic repository and adding a considerable amount of useless complexity (useless in the sense that I don't need that to enable mocking my DbContext), I'm making my DbContext-derived class implement my IUnitOfWork interface:

CallAssistantContext class

[DbConfigurationType(typeof(MySqlEFConfiguration))]
public class CallAssistantContext : DbContext, IUnitOfWork
{
    public CallAssistantContext(string connectionString)
        : base(connectionString)
    { }

    public IDbSet<TEntity> Repository<TEntity>() where TEntity : class
    {
        return base.Set<TEntity>();
    }

    public IDbSet<CustomerServiceCall> CustomerServiceCalls { get; set; }

    public IDbSet<CallType> CallTypes { get; set; }
    public IDbSet<ContactType> ContactTypes { get; set; }
    public IDbSet<IssueType> IssueTypes { get; set; }

    public IDbSet<Customer> Customers { get; set; }
    public IDbSet<CustomerStore> CustomerStores { get; set; }
    public IDbSet<SalesRep> SalesReps { get; set; }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);
    }
}

---

The "client code" depends on an abstraction - the IUnitOfWork interface, and can very easily be constructor-injected with a mock implementation in unit tests:

public class MainWindowViewModel : ViewModelBase
{
    private readonly IUnitOfWork _context;

    public MainWindowViewModel(IUnitOfWork context)
    {
        _context = context;
    }

Querying the context doesn't look too complicated either:

var cutoffDate = DateTime.Now.Subtract(TimeSpan.FromDays(7));
ServiceCalls = _context.Repository<CustomerServiceCall>()
                        .Where(e => e.CallStart >= cutoffDate)
                        .ToList();

---

So, is anything blatantly wrong with this approach? Here I've injected the UoW/context directly where it's needed - in a bigger project I would have created a "service" and constructor-injected the UoW there instead.

Answer 1

I'd second TopinFrassi's answer, that this is a more or less sensible approach, and the implementation is excellent. However, there are some potential issues, which I'll expand from craftworkgames' comments and your response. Because I can't really find anything that needs criticism in how it's written, this will be entirely design focussed.

There are two central issue in passing an interface that claims it can provide

IDbSet<TEntity> Repository<TEntity>() where TEntity : class;

• You're promising an awful lot
• You're exposing this outside of directly data-access related code (even if it's in service classes, these classes are unlikely to all be directly DA related except perhaps in a very small application)

And the problems this can cause are:

1. Exposing much more than a consumer needs. Any class that only needs to deal with SalesReps shouldn't also be passed a way to perform arbritrary crud operations on Customers. This is a violation of the Interface Segregation Principle.
2. Difficulty of mocking. An interface that can provide a lot means a lot to mock. It's likely that you'll be able to narrow down what actually needs to get mocked in individual tests, but often the practicality of that is having to couple to implementation details of the SUT which you aren't really trying to test, which makes your tests more brittle.
3. A leaky abstraction. Abstractions are about more than just being able to swap implementations, which means even if you have no interest in doing that, a leaky abstraction is still a problem. In this case you're exposing the big EF gotcha to every class who ever has to deal with entities: IQueryable. It does a good impression of IEnumerable, but then throws exceptions for a lot of arguments to its LINQ extensions which IEnumerable would happily process. Having to remember that code will need to be converted into SQL is not an issue you should expose throughout your service layer.
4. A lack of entity-specific methods. In addition to giving all your service classes too many general methods, you may also be giving them not enough specific methods. It's quite likely that you'll have queries or commands for a particular entity type which you'll want to do in several places- including across several service classes- but which are long/complex enough that repeating the entire query or command would be a DRY violation.

Resolving

That last problem is the one that screams out most for a particular answer, so let's start out with that. Say we've written a service class with a private method that does a complex query for us. Now we're writing another service class, and though the functionality isn't that similar, we find ourrselves needing the same query that was in that private method.

We do the obvious thing and pull the query out into its own class:

public class FirstFooService()
{
    public FirstFooService(IUnitOfWork unitOfWork, FooQueryHelper queryHelper) { //... }

    //...
}

public class SecondFooService()
{
    public SecondFooService(IUnitOfWork unitOfWork, FooQueryHelper queryHelper) { //... }

    //...
}

But now this really seems like a mess. We're relying on two different things- the unit of work and the query helper- to do our data access, at two different levels of abstraction. Which level do we actually want?

Well, I think the answer is straightforward. We added FooQueryHelper specifically because we found ourselves wanting that abstraction over accessing the IUnitOfWork directly. Moreover, if we look at the problems above, FooQueryHelper can solve all four of them. It solves 1,2 and 4 by its nature, and it can simply solve 3 by keeping its return types as IEnumerable rather than IQueryable, and by not exposing any methods which take an arbitrary Func or Expression to use as a filter.

All that remains is to extract the IUnitOfWork methods that we consume onto FooQueryHelper, narrowing them and making them more specific to keep them at the right abstraction level when possible. You can probably see the punchline coming- what we actually end up with isn't FooQueryHelper, but FooRepository. This can follow the standard generic repository pattern you'll find described all over the place. If you look back at the root problems, you can see these are solved too: now everything relies on a much smaller, more targeted interface, and data access concerns are kept in data access classes, rather than spreading out to every service that cares about entities.

Conclusion

But what about KISS? Possibly you were already aware of the generic repository, and you wanted to avoid it because of its apparently needless complexity and indirection. Well, yes, as always there's the tradeoff- in a small, simple application, at what point does further SOLIDifying your code consume more development resources than it'll ever buy you back? That's a judgement call you'll have to make. But hopefully I've given some reasons beyond just "So I can swap my ORM" for why you might pick this more complex pattern.

Also note that the generic repository is, at its core, not actually all that complex or bulky to implement. It really starts out as just a thin wrapper over IDbContext. But don't be misled! A repository should not just be an adapter from IDbSet to an interface you control, it should be a full-blown abstraction layer! So if you do find yourself adding more methods, that's probably vindicating the choice of that pattern.

Answer 2

Your code seems really good to me, I don't have anything else to say but that I'd remove the OnModelCreating override, as it has no purposes.