6
\$\begingroup\$

I'm looking for feedback on a repository I set up based on a blog post I read here.

I have just recently got more into .NET within the past year, so any feedback and/or best practices that can be thrown my way would be appreciated.

The branch I am working in can be seen here if interested.

IGenericRepository:

public interface IGenericRepository<T> where T : class
{
    IQueryable<T> AsQueryable();

    IEnumerable<T> GetAll();
    IEnumerable<T> Find(Expression<Func<T, bool>> predicate);
    T Single(Expression<Func<T, bool>> predicate);
    T SingleOrDefault(Expression<Func<T, bool>> predicate);
    T First(Expression<Func<T, bool>> predicate);
    T FirstOrDefault(Expression<Func<T, bool>> predicate);
    T GetById(int id);

    void Add(T entity);
    void Delete(T entity);
    void Attach(T entity);
}

IScrapedRepository:

public interface IScrapedRepository<T> : IGenericRepository<T> where T : class
{
    bool IsRefreshRequired();
    void RefreshData();
}

IUnitOfWork:

public interface IUnitOfWork : IDisposable
{
    IGenericRepository<OpeningOption> OpeningOptionRepository { get; }
    IGenericRepository<Location> LocationRepository { get; }

    void Commit();
}

EFGenericRepository:

public class EfGenericRepository<T> : IGenericRepository<T> where T : class
{
    public readonly IDbSet<T> _dbSet;

    public EfGenericRepository(IDbSet<T> dbSet)
    {
        _dbSet = dbSet;
    }

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

    public IEnumerable<T> GetAll()
    {
        return _dbSet;
    }

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

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

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

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

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

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

    public void Add(T entity)
    {
        _dbSet.Add(entity);
    }

    public void Delete(T entity)
    {
        _dbSet.Remove(entity);
    }

    public void Attach(T entity)
    {
        _dbSet.Attach(entity);
    }
}

ScrapedRepository:

public class ScrapedRepository<T> : EfGenericRepository<T>, IScrapedRepository<T> where T : class 
{
    public ScrapedRepository(IDbSet<T> dbSet) : base(dbSet) 
    {
        if (this.IsRefreshRequired()) 
        {
            this.RefreshData();
        }
    }

    public virtual bool IsRefreshRequired()
    {
        return true;
    }

    public virtual void RefreshData()
    {
    }
}

LocationRepository:

public class LocationRepository : ScrapedRepository<Location>
{
    public LocationRepository(IDbSet<Location> dbSet) : base(dbSet) { } 

    public override bool IsRefreshRequired()
    {
        //check site for new data
    }

    public override void RefreshData()
    {
        //scrape data and store it
    }
}

EfUnitOfWork:

public class EfUnitOfWork : DbContext, IUnitOfWork
{
    private readonly OpeningOptionRepository _openingOptionRepo;
    private readonly LocationRepository _locationRepo;

    public DbSet<Entities.OpeningOption> OpeningOptions { get; set; }
    public DbSet<Entities.Location> Locations { get; set; }

    public EfUnitOfWork()
    {
        _openingOptionRepo = new OpeningOptionRepository(OpeningOptions);
        _locationRepo = new LocationRepository(Locations);
    }

    public IGenericRepository<Entities.OpeningOption> OpeningOptionRepository
    {
        get { return _openingOptionRepo; }
    }

    public IGenericRepository<Entities.Location> LocationRepository
    {
        get { return _locationRepo; }
    }

    public void Commit()
    {
        this.SaveChanges();
    }
}

From there, I register IUnitOfWork with Simple Injector:

public class RepositorySetup
{
    public static void ConfigureInjections(Container container) 
    {
        container.Register<IDbContext, DaycareSearchEntities>(Lifestyle.Transient);
        container.Register<IUnitOfWork, EfUnitOfWork>(Lifestyle.Transient);
    }
}

I then use a BaseController to gain access to EFUnitOfWork in all my controllers:

public abstract class BaseController : ApiController
{
    protected IUnitOfWork UnitOfWork { get; set; }

    protected BaseController(IUnitOfWork unitOfWork)
    {
        this.UnitOfWork = unitOfWork;
    }

    protected IHttpActionResult Result<T>(T entity){
        return entity == null
            ? (IHttpActionResult)NotFound()
            : (IHttpActionResult)Ok<T>(entity)
        ;
    }
}

Then here is how I access my data in the controller:

public class OpeningOptionController : BaseController
{
    public OpeningOptionController(IUnitOfWork unitOfWork) : base(unitOfWork) { }

    [HttpGet]
    public IEnumerable<OpeningOption> GetAllOpeningOptions()
    {
        return UnitOfWork.OpeningOptionRepository.GetAll();
    }

    [HttpGet]
    public IHttpActionResult GetOpeningOption(int id)
    {
        var openingOption = UnitOfWork.OpeningOptionRepository.FirstOrDefault(o => o.ID == id);
        return Result<OpeningOption> (openingOption);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Do queries need to be part of the uow at all? There will be no updates to save. \$\endgroup\$ – Thomas Eyde Oct 6 '15 at 19:47
2
\$\begingroup\$

Overall this looks quite good.

  1. I would rename IGenericRepository to IRepository, I don't think Generic adds readability.
  2. I don't like exposing IQueryable in a public interface, it is a leaky abstraction. You don't know when database server will execute the sql.
  3. I would not have my UnitOfWork own repositories because it is difficult to test/mock.
\$\endgroup\$
  • \$\begingroup\$ I like point 2. Keep IQueryable in the service layer where it belongs. Else developers will start taking advantage of it (in the wrong way) and before you know it, it will be too late to take it away. \$\endgroup\$ – Vince Panuccio Aug 28 '15 at 10:42
0
\$\begingroup\$

The patterns here seem well refined. The only thing I would change is, depending on the size and complexity of the application, is to refactor the IUnitOfWork/Repo interaction out of the controller and into a service layer.

I'm personally working on a way to eliminate the IUnitOfWork out all together using some kind of factory.

Edit:

Here is an example snippet from my attached question. This is what I'd want to replace the IUnitOfWork with.

namespace EFDataAccess
{
    class RepositoryFactory
    {

        private readonly DbContext _context;

        public RepositoryFactory(DbContext context)
        {
            _context = context;
        } 

        public IGenericRepository<T> Get<T>() where T : Type
        {
            return new EFGenericRepository<T>(_context.Set<T>());
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview, solomon8718. Would you expand on why you'd refactor IUnitOfWork? \$\endgroup\$ – Legato Apr 20 '15 at 12:25
  • \$\begingroup\$ I would definitely introduce a service layer as the app progresses and business logic comes to exist. Right now, I basically stubbed in the baseline data access. That said, I still planned to use UnitOfWork directly for simple operations that can be achieved directly through EF. I am interested to see what you meant by the factory that eliminates the UnitOfWork. \$\endgroup\$ – Scott Smith Apr 20 '15 at 13:28
  • \$\begingroup\$ I'm still working out the code to get it to work, but from my understanding the IUnitOfWork in this instance seems to be a type of sudo factory. A single point of access to all the Repositories with each repository type hardcoded with accessors to each. My goal is to make this as dynamic as possible. \$\endgroup\$ – MplsAmigo Apr 20 '15 at 13:59
  • \$\begingroup\$ See here for an example of what I'm talking about: codereview.stackexchange.com/questions/87437/… \$\endgroup\$ – MplsAmigo Apr 20 '15 at 14:19
  • \$\begingroup\$ What happens when you have the need for some custom methods on one or more repositories? You can extend EFGenericRepository, but then you can't get at it from your RepositoryFactory (which lets face it, is the nearly the same as this UnitOfWork class renamed with a method for getting repositories using generics). I think when we talked about this initially we already discussed this road block. \$\endgroup\$ – Scott Smith Apr 21 '15 at 1:31

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.