Unit of Work with Repository Pattern MVC 5 & EF 6

Question

I put together a sample of how I am using the Unit of Work & Repository pattern based on my understanding. Can anyone please let me know if I am implementing this the correct way? If I am not, how can I improve it?

Thanks in advance, it's much appreciated.

I have an EF model with two entities: Topic and Subtopic. The EF model is called CommonGood.

Unit of Work:

/// <summary>
/// Implementation of a UnitOfWork class
/// </summary>
public static class UnitOfWork
{
    /// <summary>
    /// Gets the default context
    /// </summary>
    /// <returns>A new instance of the default context</returns>
    public static CommonGoodEntities GetContext()
    {
        return new CommonGoodEntities();
    }
}

IGenericRepository:

public interface IRepository<T>
{
    /// <summary>
    /// Gets all entities
    /// </summary>        
    /// <returns>All entities</returns>
    IEnumerable<T> GetAll();

    /// <summary>
    /// Gets all entities matching the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>All entities matching the predicate</returns>
    IEnumerable<T> GetAll(Expression<Func<T, bool>> predicate);

    /// <summary>
    /// Set based on where condition
    /// </summary>
    /// <param name="predicate">The predicate</param>
    /// <returns>The records matching the given condition</returns>
    IQueryable<T> Where(Expression<Func<T, bool>> predicate);

    /// <summary>
    /// Finds an entity matching the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>An entity matching the predicate</returns>
    IEnumerable<T> Find(Expression<Func<T, bool>> predicate);

    /// <summary>
    /// Determines if there are any entities matching the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>True if a match was found</returns>
    bool Any(Expression<Func<T, bool>> predicate);

    /// <summary>
    /// Returns the first entity that matches the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>An entity matching the predicate</returns>
    T First(Expression<Func<T, bool>> predicate);

    /// <summary>
    /// Returns the first entity that matches the predicate else null
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>An entity matching the predicate else null</returns>
    T FirstOrDefault(Expression<Func<T, bool>> predicate);

    /// <summary>
    /// Adds a given entity to the context
    /// </summary>
    /// <param name="entity">The entity to add to the context</param>
    void Add(T entity);

    /// <summary>
    /// Deletes a given entity from the context
    /// </summary>
    /// <param name="entity">The entity to delete</param>
    void Delete(T entity);

    /// <summary>
    /// Attaches a given entity to the context
    /// </summary>
    /// <param name="entity">The entity to attach</param>
    void Attach(T entity);
}

Generic Repository:

public class GenericRepository<T> : IRepository<T> where T : class
{
    /// <summary>
    /// The database context for the repository
    /// </summary>
    private DbContext _context;

    /// <summary>
    /// The data set of the repository
    /// </summary>
    private IDbSet<T> _dbSet;

    /// <summary>
    /// Initializes a new instance of the <see cref="GenericRepository{T}" /> class.        
    /// </summary>
    /// <param name="context">The context for the repository</param>        
    public GenericRepository(DbContext context)
    {
        this._context = context;
        this._dbSet = this._context.Set<T>();
    }

    /// <summary>
    /// Gets all entities
    /// </summary>        
    /// <returns>All entities</returns>
    public IEnumerable<T> GetAll()
    {
        return this._dbSet;
    }

    /// <summary>
    /// Gets all entities matching the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>All entities matching the predicate</returns>
    public IEnumerable<T> GetAll(System.Linq.Expressions.Expression<Func<T, bool>> predicate)
    {
        return this._dbSet.Where(predicate);
    }

    /// <summary>
    /// Set based on where condition
    /// </summary>
    /// <param name="predicate">The predicate</param>
    /// <returns>The records matching the given condition</returns>
    public IQueryable<T> Where(Expression<Func<T, bool>> predicate)
    {
        return this._dbSet.Where(predicate);
    }

    /// <summary>
    /// Finds an entity matching the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>An entity matching the predicate</returns>
    public IEnumerable<T> Find(System.Linq.Expressions.Expression<Func<T, bool>> predicate)
    {
        return this._dbSet.Where(predicate);
    }

    /// <summary>
    /// Determines if there are any entities matching the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>True if a match was found</returns>
    public bool Any(Expression<Func<T, bool>> predicate)
    {
        return this._dbSet.Any(predicate);
    }

    /// <summary>
    /// Returns the first entity that matches the predicate
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>An entity matching the predicate</returns>
    public T First(Expression<Func<T, bool>> predicate)
    {
        return this._dbSet.First(predicate);
    }

    /// <summary>
    /// Returns the first entity that matches the predicate else null
    /// </summary>
    /// <param name="predicate">The filter clause</param>
    /// <returns>An entity matching the predicate else null</returns>
    public T FirstOrDefault(Expression<Func<T, bool>> predicate)
    {
        return this._dbSet.FirstOrDefault(predicate);
    }

    /// <summary>
    /// Adds a given entity to the context
    /// </summary>
    /// <param name="entity">The entity to add to the context</param>
    public void Add(T entity)
    {
        this._dbSet.Add(entity);
    }

    /// <summary>
    /// Deletes a given entity from the context
    /// </summary>
    /// <param name="entity">The entity to delete</param>
    public void Delete(T entity)
    {
        this._dbSet.Remove(entity);
    }

    /// <summary>
    /// Attaches a given entity to the context
    /// </summary>
    /// <param name="entity">The entity to attach</param>
    public void Attach(T entity)
    {
        this._dbSet.Attach(entity);
    }
}

Controller:

public class HomeController : Controller
{
    /// <summary>
    /// The context used for the controller
    /// </summary>
    private DbContext _context;

    /// <summary>
    /// Initializes a new instance of the <see cref="HomeController"/> class.
    /// </summary>        
    public HomeController()
    {
        this._context = UnitOfWork.GetContext();
    }

    public JsonResult GetTopics()
    {
        var topics = new GenericRepository<Topic>(this._context).GetAll().ToList();            
        return this.Json(topics, JsonRequestBehavior.AllowGet);
    }

    /// <summary>
    /// Disposes of the context if the currently disposing
    /// </summary>
    /// <param name="disposing">A value indicating whether or not the application is disposing</param>
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            this._context.Dispose();
        }

        base.Dispose(disposing);
    }
}

Essentially I want to make sure I am accessing data in the proper way and making sure I am not overlooking anything. Again, thanks!

Solution

It's not perfect to implement UnitOfWork as a static class. Define an interface IUnitOfWork. Your db context will implement this interface. It may look like:

public interface IUnitOfWork {
    int SaveChanges();
}

public class EFDbContext: DbContext, IUnitOfWork {

    public DbSet<User> User { get; set; }

    public EFDbContext(string connectionString)
        : base(connectionString) { }

    public override int SaveChanges() {
        return base.SaveChanges();
    }
}

I usually create several repositories inherited from generic repository. Thus names of repository find methods could have more specific names. This also prevents from duplication of repository logic in different controllers.

For example:

    public class EFUserRepository: EFRepository<User>, IUserRepository {

    public EFUserRepository(IUnitOfWork context)
        : base(context) { }

    protected override DbSet<User> Table {
        get { return Context.User; }
    }

    public User Find(string email) {
        return Table.FirstOrDefault(u => u.Email == email);
    }

    public bool Validate(string email, string password) {
        string passwordHash = Cryptography.GenerateHash(password);
        User user = Find(email);
        return user != null && user.Password == passwordHash;
    }

Now about controller: To simplify testing it's better to use IoC Container, e.g. NInject So the dependencies between controller <-> repo <-> unitOfWork will be resolved by NInject.

This how could look UserController with Login method:

public class UserController: Controller {

    [Ninject.Inject]
    public IUserRepository UserRepository { get; set; }

    public ActionResult Login(AuthorizationViewModel vm) {
        if(ModelState.IsValid) {
            if(UserRepository.Validate(vm.Email, vm.Password)) {
                FormsAuthentication.SetAuthCookie(vm.Email, true);
                if(Url.IsLocalUrl(vm.ReturnUrl)) {
                    return Redirect(vm.ReturnUrl);
                }
                else {
                    return RedirectToAction("Page", "Main");
                }
            }
            else {
                ModelState.AddModelError("", Resources.Validation.WrongEmailOrPassword);
            }
        }

        return View(vm);
    }
}

The dependencies resolving could be done by custom controller factory, like this:

public class NInjectControllerFactory: DefaultControllerFactory {
    public IKernel Kernel { get; private set; }

    public NInjectControllerFactory() {
        Kernel = new StandardKernel();
        AddBindings();
    }

    protected override IController GetControllerInstance(System.Web.Routing.RequestContext requestContext, Type controllerType) {
        return controllerType == null
            ? null
            : (IController)Kernel.Get(controllerType);
    }

    private void AddBindings() {
        Kernel.Bind<IUnitOfWork>().To(typeof(EFDbContext)).InRequestScope();
        Kernel.Bind<IUserRepository>().To(typeof(EFUserRepository).InRequestScope();
    }
}

And replace current controller factory with your custom. You can do it in Application_Start handler of Global.asax:

protected void Application_Start() {
    ...
ControllerBuilder.Current.SetControllerFactory(new NInjectControllerFactory());

}

If you decide to use NInject, you can simply add it with Nuget. To enable binding InRequestScope you also need NInject.Web.Common. Of course, there a lot of other alternatives like Castle Windsor or StructureMap, but NInject is the simplest one.

Hope it will help.

Comments

[Jace Rhea]

You may want to try codereview.stackexchange.com. This isn't really a question for SO.

[Gert Arnold]

This question appears to be off-topic because it is more fit for CodeReview

[Jeremy Todd]

Can you explain the benefits of using your IUnitOfWork interface in this example (as opposed to passing a plain EFDbContext)? I've seen (and used) this pattern when the IUnitOfWork interface provides abstracted querying or change-tracking methods (in order to avoid coupling with EF classes directly), but the way you're implementing it you need to have access to an IUnitOfWork and an EFDbContext instance -- you still have the tight coupling, but now you also have to deal with an extra interface that provides no functionality you can't already access through your EFDbContext.

[tabalin]

As you've said, it lets avoid coupling between EF and repositories. In my case, I have two types of repos for EF and Azure Table Storage. That is why I really need this decoupling. In current case, you are right, we can do without IUnitOfWork. Anyway less coupling in common scenario better.