2
\$\begingroup\$

I'm implementing a generic repository + UoW pattern with EF6 and simple injector. I feel there is room for improvements. Please let me know if I left out some relevant code, I tried to keep it breif.

Do you see any need for the unit of work to keep track of the repositories (in its _repositories field)? As you can see I'm currently not using it.

Another important thing here is that I want one EF context, LGTContext, per web request. But as far as I understand, it would be the case due to the Lifestyle.Scoped registration lifestyle.

Any other thoughts?

Generic repository:

public abstract class DatabaseRepository<T> : IRepository<T> where T : BaseEntity
{
    protected readonly DbSet<T> DbSet;
    protected readonly LGTContext Context;

    protected DatabaseRepository(IUnitOfWork unitOfWork, LGTContext context)
    {
        Context = context;
        DbSet = Context.Set<T>();

        unitOfWork.Register(this);
    }

    public Task<PagedResult<T>> Find(Expression<Func<T, bool>> predicate, int pageNumber, int pageSize)
    {
        throw new NotImplementedException();
    }

    public virtual async Task<IEnumerable<T>> Find(Expression<Func<T, bool>> predicate)
    {
        return await DbSet.Where(predicate).ToListAsync();
    }

    public virtual async Task<IEnumerable<T>> GetAll()
    {
        return await DbSet.ToListAsync();
    }

    public virtual async Task<T> GetSingle(object id)
    {
        return await DbSet.FindAsync(id);
    }

    public virtual void Add(T entity)
    {
        DbSet.Add(entity);
    }

    public virtual void Delete(T entity)
    {
        if (Context.Entry(entity).State == EntityState.Detached)
        {
            DbSet.Attach(entity);
        }
        DbSet.Remove(entity);
    }

    public virtual void Edit(T entity)
    {
        DbSet.Attach(entity);
        Context.Entry(entity).State = EntityState.Modified;
    }
}

Unit of work:

public class UnitOfWork : IUnitOfWork, IDisposable
{  
    private readonly Dictionary<Type, IRepository> _repositories = new Dictionary<Type, IRepository>();
    private readonly LGTContext _context;
    private bool _disposed;

    public UnitOfWork(LGTContext context)
    {
        _context = context;
    }

    public async Task CommitAsync()
    {
        try
        {
            await _context.SaveChangesAsync();
        }
        catch (DbEntityValidationException e)
        {
            // todo: handle validation errors...
            throw;
        }
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                _context.Dispose();
            }
        }
        _disposed = true;
    }

    public void Register(IRepository repository)
    {
        _repositories.Add(repository.GetType(), repository);
    }
}

Simple injector registration:

container.Register<IUnitOfWork, UnitOfWork>(Lifestyle.Scoped);
container.Register(typeof(IRepository<>), 
                        new[] { typeof(DatabaseRepository<>).Assembly }, Lifestyle.Scoped);
container.Register<LGTContext>(Lifestyle.Scoped);

Usage psuedo code:

public SomeController(IUnitOfWork uow, IRepository<SomeEntity> repo)
{
    var entities = await repo.GetAll();
    // modify some entities...
    await uow.CommitAsync();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Did you know that Entify Framework already implements the UoW pattern? On the other side building UoW on top of UoW is cheating but I can understand it because there is not a single example on the Internet explaining it without EF. Just use a repository to encapsulate your queries any you'll be fine. \$\endgroup\$ – t3chb0t May 16 '17 at 20:19
  • \$\begingroup\$ @t3chb0t yes, but then any code using context.savechanges() wouldn't be testable, which I consider a downside. \$\endgroup\$ – Johan May 16 '17 at 20:41
  • 1
    \$\begingroup\$ All of the asynchronous methods should be named with the Async suffix, not just some of them. Also, make DbSet and Context read-only properties rather than fields (C#6). \$\endgroup\$ – Jesse C. Slicer May 17 '17 at 15:32
  • \$\begingroup\$ @JesseC.Slicer that's a completely valid review. You should make it an answer instead of leaving it in the comments. \$\endgroup\$ – RubberDuck Jun 17 '17 at 11:46
1
\$\begingroup\$

Putting aside the fact that EF already provides both a UoW and Generic Repository, there's still a huge flaw with your UnitOfWork. It disposes a resource it doesn't own. You pass the context in as a constructor arg, store it as a member, and then code analysis yelled at you for not implementing IDisposable, so you disposed of it. The problem is, the UoW has no way of knowing that it's supposed to dispose of the context, because it was injected into the class.

In all the years I've been working with .Net, not once have I actually needed to implement IDisposable. Every single time that I thought I needed to, I didn't. I just needed to keep the lifetime of the IDisposable short and contained within a using block. That's possible if you abandon this folly and implement a domain specific service instead.

public class EFWidgetStore : IWidgetStore
{
    private readonly Func<DbContext> GetContext;
    public EFWidgetStore(Func<DbContext> contextFactory)
    {
        GetContext = contextFactory;
    }

    public Widget Get(int id)
    {
        using (context = GetContext())
        {
            //... get it
        }
    }

    public Widget Get(string name)
    {
         //...
    }

Next up, you've broken Liskov's substitution principle, which is common for these kinds of implementations.

public Task<PagedResult<T>> Find(Expression<Func<T, bool>> predicate, int pageNumber, int pageSize)
{
    throw new NotImplementedException();
}

Although, let's be honest. When will you ever treat a specific repository as an instance of the base class anyway... You've introduced a world of hurt on yourself for no gain what so ever. Please don't do this.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I agree that it feels a bit redundant to just proxy the methods the EF repo and uow. My reason is that I want the code to be testable. As far as I can tell, a class with a concrete dependency of the EF context is pretty hard to unit test, right? "The problem is, the UoW has no way of knowing that it's supposed to dispose of the context, because it was injected into the class.". Really? I thought the IoC container took care of this, due to its registration with a scoped lifetime, it should be automatically disposed at the end of each request. \$\endgroup\$ – Johan May 17 '17 at 6:00
  • \$\begingroup\$ There's nothing to test! It just passes the call onto EF. It's completely anemic. Give your things Fake IWidgetStores. Why does your UoW assume there's an IoC container? It really shouldn't. Tell you what, just try writing your persistence layer like I showed you. If you don't like it, throw it away. \$\endgroup\$ – RubberDuck May 17 '17 at 9:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.