60
\$\begingroup\$

I am about to >explode< :) due to amount of reading on this subject... My head hurts, and I need some honest opinions... There is a similar question/review that I noticed, but I believe my approach is bit different so I wanted to ask.

We are embarking on a really big project that will use ASP.NET MVC 4 with Web API and would like to use Entity Framework Code First. Before I post code for review, please consider following

  • My project will be big (possibly 100+ domain entities)
  • There needs to SOLID approach to architecture
  • By now, I understand that Entity Framework DbContext is (entirely or to some extent) implementing Unit of Work and Repository patterns, so I could skip all this ;)
  • I am really concerned using EF injected directly into Controller constructors (testability, separation of concerns, etc, etc)
  • I am also aware that there is more than one way to implement UoW and Repository patterns.
  • I am not worried or wanting to build abstractions that will allow me to "swap" ORM (e.g. swap Entity Framework for NHiberante or such).

My Approach

  • Repositories are generic and have a base class that implement much of the standard logic.
  • Repositories need DbContext via constructor (which is provided by UnitOfWork)
  • UnitOfWork is responsible for managing access to all repositories and insuring context is shared among them.
  • UnitOfWork is disposable, Repositories are not...
  • In order to "hide" DbContext, UnitOfWork is constructed using IDbContextFactory.

Questions

  • This seems to work for me, and the advantage I see is that every Controller just needs UoW injected which is nice. Some Controllers need 2-3 repositories in addition to domain services so this makes things nice...I think...
  • Over time, UoW will grow with repositories (there could be 65+ aggregate roots each having a repo). Any ides on how to better manage this? Should I somehow inject Repositories instead od new()-ing them up in the UnitOfWork? I'd love to be able to create a IoC module (Autofac is my poison) to wire up all repos (somehow)
  • Is use of IDbContextFactory an overkill or should I just inject DbContext to constructor of UnitOfWork instead? Right now, my web app has no direct dependency on Entity Framework it only depends n DAL (which in turn depends on EF). On the other hand DbContextFactory new()es up MyAppDbContext and is not handled by IoC
  • Does anyone notices any other "code smell" ?
  • Some of the questions are in code NOTEs to make them more relevant...

Ok here is the code with 2 repositories and sample use (all namespaces are omitted for brevity sake)

IDbContextFactory and DbContextFactory

/// <summary>
/// Creates instance of specific DbContext
/// </summary>
public interface IDbContextFactory //: IDisposable  //NOTE: Since UnitOfWork is disposable I am not sure if context factory has to be also...
{
    DbContext GetDbContext();
}

public class DbContextFactory : IDbContextFactory
{
    private readonly DbContext _context;

    public DbContextFactory()
    {
        // the context is new()ed up instead of being injected to avoid direct dependency on EF
        // not sure if this is good approach...but it removes direct dependency on EF from web tier
        _context = new MyAppDbContext(); 
    }

    public DbContext GetDbContext()
    {
        return _context;
    }

    // see comment in IDbContextFactory inteface...
    //public void Dispose()
    //{
    //    if (_context != null)
    //    {
    //        _context.Dispose();
    //        GC.SuppressFinalize(this);
    //    }
    //}
}

IRepository, Repository, and 2 specific Repositories wiht extra interfaces (Vehicle and Inventory)

public interface IRepository<T> where T : class
{
    /// <summary>
    ///   Get the total objects count.
    /// </summary>
    int Count { get; }

    /// <summary>
    ///   Gets all objects from database
    /// </summary>
    IQueryable<T> All();

    /// <summary>
    ///   Gets object by primary key.
    /// </summary>
    /// <param name="id"> primary key </param>
    /// <returns> </returns>
    T GetById(object id);

    /// <summary>
    ///   Gets objects via optional filter, sort order, and includes
    /// </summary>
    /// <param name="filter"> </param>
    /// <param name="orderBy"> </param>
    /// <param name="includeProperties"> </param>
    /// <returns> </returns>
    IQueryable<T> Get(Expression<Func<T, bool>> filter = null, Func<IQueryable<T>, IOrderedQueryable<T>> orderBy = null, string includeProperties = "");

    /// <summary>
    ///   Gets objects from database by filter.
    /// </summary>
    /// <param name="predicate"> Specified a filter </param>
    IQueryable<T> Filter(Expression<Func<T, bool>> predicate);

    /// <summary>
    ///   Gets objects from database with filting and paging.
    /// </summary>
    /// <param name="filter"> Specified a filter </param>
    /// <param name="total"> Returns the total records count of the filter. </param>
    /// <param name="index"> Specified the page index. </param>
    /// <param name="size"> Specified the page size </param>
    IQueryable<T> Filter(Expression<Func<T, bool>> filter, out int total, int index = 0, int size = 50);

    /// <summary>
    ///   Gets the object(s) is exists in database by specified filter.
    /// </summary>
    /// <param name="predicate"> Specified the filter expression </param>
    bool Contains(Expression<Func<T, bool>> predicate);

    /// <summary>
    ///   Find object by keys.
    /// </summary>
    /// <param name="keys"> Specified the search keys. </param>
    T Find(params object[] keys);

    /// <summary>
    ///   Find object by specified expression.
    /// </summary>
    /// <param name="predicate"> </param>
    T Find(Expression<Func<T, bool>> predicate);

    /// <summary>
    ///   Create a new object to database.
    /// </summary>
    /// <param name="entity"> Specified a new object to create. </param>
    T Create(T entity);

    /// <summary>
    ///   Deletes the object by primary key
    /// </summary>
    /// <param name="id"> </param>
    void Delete(object id);

    /// <summary>
    ///   Delete the object from database.
    /// </summary>
    /// <param name="entity"> Specified a existing object to delete. </param>
    void Delete(T entity);

    /// <summary>
    ///   Delete objects from database by specified filter expression.
    /// </summary>
    /// <param name="predicate"> </param>
    void Delete(Expression<Func<T, bool>> predicate);

    /// <summary>
    ///   Update object changes and save to database.
    /// </summary>
    /// <param name="entity"> Specified the object to save. </param>
    void Update(T entity);
}

public class Repository<T> : IRepository<T> where T : class
{
    protected readonly DbContext _dbContext;
    protected readonly DbSet<T> _dbSet;

    public Repository(DbContext dbContext)
    {
        _dbContext = dbContext;
        _dbSet = _dbContext.Set<T>();
    }

    public virtual int Count
    {
        get { return _dbSet.Count(); }
    }

    public virtual IQueryable<T> All()
    {
        return _dbSet.AsQueryable();
    }

    public virtual T GetById(object id)
    {
        return _dbSet.Find(id);
    }

    public virtual IQueryable<T> Get(Expression<Func<T, bool>> filter = null, Func<IQueryable<T>, IOrderedQueryable<T>> orderBy = null, string includeProperties = "")
    {
        IQueryable<T> query = _dbSet;

        if (filter != null)
        {
            query = query.Where(filter);
        }

        if (!String.IsNullOrWhiteSpace(includeProperties))
        {
            foreach (var includeProperty in includeProperties.Split(new[] {','}, StringSplitOptions.RemoveEmptyEntries))
            {
                query = query.Include(includeProperty);
            }
        }

        if (orderBy != null)
        {
            return orderBy(query).AsQueryable();
        }
        else
        {
            return query.AsQueryable();
        }
    }

    public virtual IQueryable<T> Filter(Expression<Func<T, bool>> predicate)
    {
        return _dbSet.Where(predicate).AsQueryable();
    }

    public virtual IQueryable<T> Filter(Expression<Func<T, bool>> filter, out int total, int index = 0, int size = 50)
    {
        int skipCount = index*size;
        var resetSet = filter != null ? _dbSet.Where(filter).AsQueryable() : _dbSet.AsQueryable();
        resetSet = skipCount == 0 ? resetSet.Take(size) : resetSet.Skip(skipCount).Take(size);
        total = resetSet.Count();
        return resetSet.AsQueryable();
    }

    public bool Contains(Expression<Func<T, bool>> predicate)
    {
        return _dbSet.Count(predicate) > 0;
    }

    public virtual T Find(params object[] keys)
    {
        return _dbSet.Find(keys);
    }

    public virtual T Find(Expression<Func<T, bool>> predicate)
    {
        return _dbSet.FirstOrDefault(predicate);
    }

    public virtual T Create(T entity)
    {
        var newEntry = _dbSet.Add(entity);
        return newEntry;
    }

    public virtual void Delete(object id)
    {
        var entityToDelete = _dbSet.Find(id);
        Delete(entityToDelete);
    }

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

    public virtual void Delete(Expression<Func<T, bool>> predicate)
    {
        var entitiesToDelete = Filter(predicate);
        foreach (var entity in entitiesToDelete)
        {
            if (_dbContext.Entry(entity).State == EntityState.Detached)
            {
                _dbSet.Attach(entity);
            }
            _dbSet.Remove(entity);
        }
    }

    public virtual void Update(T entity)
    {
        var entry = _dbContext.Entry(entity);
        _dbSet.Attach(entity);
        entry.State = EntityState.Modified;
    }
}   


public class VehicleRepository : Repository<Vehicle>, IVehicleRepository
{
    public VehicleRepository(DbContext dbContext) : base(dbContext)
    {
    }

}

public interface IVehicleRepository : IRepository<Vehicle>
{
    //RFU
}   

public interface IInventoryRepository : IRepository<InventoryItem>
{
    IList<InventoryItem> GetByVehicleId(string vehicleId); // NOTE: InventoryItem.VehicleId != InventoryItem.Id
}

public class InventoryItemRepository : Repository<InventoryItem>, IInventoryItemRepository
{
    public InventoryItemRepository(DbContext dbContext) : base(dbContext)
    {
    }

    public IList<InventoryItem> GetByVehicleId(string vehicleId)
    {
        return Filter(vii => vii.Vehicle.Id == vehicleId).ToList();
    }
}  

IUnitOfWork, UnitOfWork

public interface IUnitOfWork : IDisposable
{
    InventoryItemRepository InventoryItemRepository { get; }
    VehicleRepository VehicleRepository { get; }
    void Save();
}

public class UnitOfWork : IUnitOfWork
{
    private readonly DbContext _dbContext;
    private bool _disposed;
    private InventoryItemRepository _inventoryItemRepository;
    private VehicleRepository _vehicleRepository;


    /// <summary>
    /// NOTE: repository getters instantiate repositories as needed (lazily)...
    ///       i wish I knew of IoC "way" of wiring up repository getters...
    /// </summary>
    /// <param name="dbContextFactory"></param>
    public UnitOfWork(IDbContextFactory dbContextFactory)
    {
        _dbContext = dbContextFactory.GetDbContext();
    }

    public void Save()
    {
        if (_dbContext.GetValidationErrors().Any())
        {
            // TODO: move validation errors into domain level exception and then throw it instead of EF related one
        }
        _dbContext.SaveChanges();
    }

    public InventoryItemRepository InventoryItemRepository
    {
        get { return _inventoryItemRepository ?? (_inventoryItemRepository = new InventoryItemRepository(_dbContext)); }
    }

    public VehicleRepository VehicleRepository
    {
        get { return _vehicleRepository ?? (_vehicleRepository = new VehicleRepository(_dbContext)); }
    }

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

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

Sample Usage in ASP.NET MVC4 + Web API

Global.asax.cs (Application_Start)

// relevant registration
        builder.RegisterType<UnitOfWork>().As<IUnitOfWork>()
            .WithParameter("dbContextFactory", new DbContextFactory())
            .InstancePerHttpRequest()
            .InstancePerApiRequest();

InventoryController

public class InventoryController : ApiController
{

    private readonly InventoryItemMapper _mapper; // NOTE: maps viewModel to domain entities and vice versa using ValueInjector
    private readonly IUnitOfWork _unitOfWork;

    public InventoryController(IUnitOfWork unitOfWork)
    {
        _unitOfWork = unitOfWork; // UoW (with all repos) is injected and ready for use...
        _mapper = new VehicleInventoryItemMapper(); //TODO: this will be injected also...

    }

    public IEnumerable<InventoryViewModel> Get()
    {
        var inventoryItems = _unitOfWork.InventoryItemRepository.All().ToList();
        var inventory = _mapper.MapToModel(inventoryItems);
        return inventory;

    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ I don´t like the relationship you have between the Unit of Work and the repositories. Every time you need to add a repository you need to touch the UnitOfWork class, that for me is a code smell. \$\endgroup\$ – Markust Oct 22 '12 at 23:24
  • \$\begingroup\$ @Markust I agree with you - I have been modifying my "design" since I posted this 5 months ago, and one of the things I have changed was to decouple UoW from Repositories. \$\endgroup\$ – zam6ak Oct 23 '12 at 14:40
  • \$\begingroup\$ @zam6ak How did you decouple the repos from the UOW? I have been working on a similar pattern to this and found that although I need to edit the UOW to add new repos it was still pretty clean. \$\endgroup\$ – SecretDeveloper Jan 10 '13 at 14:40
  • \$\begingroup\$ @SecretDeveloper Create a common interface, and then have ORM specific implementations. Of course the more abstract you get, the more ORM specific functionality you lose. \$\endgroup\$ – zam6ak Jan 11 '13 at 20:24
21
\$\begingroup\$

If you are likely to grow to 100+ domain objects, I'm making the assumption you will have a lot of logic.

I wouldn't access the repositories directly via the controllers, I would add a service layer that is retrieved by the controllers via the UnitOfWork.

reason being:

  • Your controller actions are likely to bloat and become unmanageable. As you will have alot of boiler code (e.g. Get, Update, Save).

  • The service layer would make you avoid from breaking the DRY principle because you won't be repeating the typical repository code.

  • You could use the Facade pattern to group common tasks that require multiple repositories http://en.wikipedia.org/wiki/Facade_pattern

  • Your business logic will be in the Service Layer, allowing them to be reused elsewhere, plus your controllers will be loosely coupled to your business logic

example I've built: https://gist.github.com/3025099

\$\endgroup\$
11
\$\begingroup\$

I would agree with @Mike reasoning of avoiding controller bloat via services.

Wrapping up DbContext is a leaky abstraction. No matter what, you'll end up in some kind of dependency on EF in your services/controller layer.

Besides that, I would avoid an extra layer of UnitOfWork and Repository simply because DbContext wraps that up for you already.

Per MSDN:

DbContext Class

Represents a combination of the Unit-Of-Work and Repository patterns and enables you to query a database and group together changes that will then be written back to the store as a unit.

If you use any DI framework, you can manage lifetime of the DbContext and the services pretty easily.

Further reading: Architecting in the pit of doom: The evils of the repository abstraction layer

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice link. I also follow Ayende's blog, and am well aware of his "position" on UoW and Repository :) You are right, EF's DbContext is already an abstraction, but I still think you should not use it in controllers directly (but via services, as you mentioned). \$\endgroup\$ – zam6ak Oct 15 '12 at 13:57
2
\$\begingroup\$

It looks good except filter for paging will through error .

The method 'Skip' is only supported for sorted input in LINQ to Entities. The method 'OrderBy' must be called before the method 'Skip'.

\$\endgroup\$
  • \$\begingroup\$ Same here.. any ideas how to fix? \$\endgroup\$ – Hades Aug 16 '13 at 22:38
0
\$\begingroup\$

It may have just been a typo, but I would probably suggest having the IUnitOfWork expose interfaces instead of the concrete implementations, e.g. IVehicleRepository instead of VehicleRepository.

I quite like the use of IDBContextFactory in this regard, it's not entirely overkill but nicely abstracts DBContext away. However upon implementing this would you not then pass the factory into the Repository<T> constructors rather than DBContext itself?

I've always wondered myself about how to ensure controllers have access to a number or repositories etc. I am interested to see other people's comments on exposing all the repositories on the UnitOfWork.

\$\endgroup\$
  • \$\begingroup\$ You would not want to pass the IDBContextFactory to each repository. Entity Framework deals with tracking changes and does this through attaching entities to its DBContext. If you do not share the DBContext created by the IDBContextFactory with each repository then you will lose this capability and will have issues when updating linked entities such as a Contact and the Contact's -> EmailAddress. It's a natural tenancy to want to create a new instance of the DBContext for each repository but I've made that mistake and trust me you don't want to do the same. \$\endgroup\$ – user17129 Oct 5 '12 at 22:02
  • \$\begingroup\$ @Curry. Yeah, dbcontexts should be shared I agree. I've seen the same problems you mention. But the use of the contextfactory seems overkill if it's not going to be used anywhere else. Otherwise dbcontext may as well just be injected when setting up the injection rules especially if using frameworks such as Unity or Ninject.... \$\endgroup\$ – dreza Oct 6 '12 at 4:58

protected by syb0rg Feb 25 '14 at 21:47

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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