Problem
I am implementing generic repository and unit of work for the first time. I would be glad if someone can correct me if I am doing something wrong here.
This is how I communicate with my DAL from controller:
public class HomeController : Controller
{
private UnitOfWork unitOfWork = new UnitOfWork();
public ActionResult Index()
{
var cities = unitOfWork.CityRepository.Get();
I instatiate unit of work in controller is this ok to it like this?
Unit of work contains all repositories so do I actually need to use ninject to inject repositories in controllers?
Suddenly when I finish this implementation I don’t see use of injection as all repositories are in object that I instatiate in each controller?
This is my DAL classes:
public interface ICityRepository
{}
public class CityRepository : GenericRepository<City>, ICityRepository
{}
public class GenericRepository<TEntity> where TEntity : class
{
internal MainContext context;
internal DbSet<TEntity> dbSet;
public GenericRepository(MainContext context)
{
this.context = context;
this.dbSet = context.Set<TEntity>();
}
public virtual IEnumerable<TEntity> Get(
Expression<Func<TEntity, bool>> filter = null,
Func<IQueryable<TEntity>, IOrderedQueryable<TEntity>> orderBy = null,
string includeProperties = "")
{
IQueryable<TEntity> query = dbSet;
if (filter != null)
{
query = query.Where(filter);
}
foreach (var includeProperty in includeProperties.Split
(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries))
{
query = query.Include(includeProperty);
}
if (orderBy != null)
{
return orderBy(query).ToList();
}
else
{
return query.ToList();
}
}
public virtual TEntity GetByID(object id)
{
return dbSet.Find(id);
}
public virtual void Insert(TEntity entity)
{
dbSet.Add(entity);
}
public virtual void Delete(object id)
{
TEntity entityToDelete = dbSet.Find(id);
Delete(entityToDelete);
}
public virtual void Delete(TEntity entityToDelete)
{
if (context.Entry(entityToDelete).State == EntityState.Detached)
{
dbSet.Attach(entityToDelete);
}
dbSet.Remove(entityToDelete);
}
public virtual void Update(TEntity entityToUpdate)
{
dbSet.Attach(entityToUpdate);
context.Entry(entityToUpdate).State = EntityState.Modified;
}
}
public class UnitOfWork : IDisposable
{
private MainContext context = new MainContext();
private ApplicationUserRepository applicationUserRepository;
private CountryRepository countryRepository;
private CityRepository cityRepository;
public ApplicationUserRepository ApplicationUserRepository
{
get
{
if (this.applicationUserRepository == null)
{
this.applicationUserRepository = new ApplicationUserRepository(context);
}
return applicationUserRepository;
}
}
public CountryRepository CountryRepository
{
get
{
if (this.countryRepository == null)
{
this.countryRepository = new CountryRepository(context);
}
return countryRepository;
}
}
public CityRepository CityRepository
{
get
{
if (this.cityRepository == null)
{
this.cityRepository = new CityRepository(context);
}
return cityRepository;
}
}
public void Save()
{
context.SaveChanges();
}
private bool disposed = false;
protected virtual void Dispose(bool disposing)
{
if (!this.disposed)
{
if (disposing)
{
context.Dispose();
}
}
this.disposed = true;
}
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
}
Solution
Yes, you need a IOC container to maintain your instances. No, your Unit of Work should not create them.
With an IOC Container, you centralize the control over the objects and their lifetime. This is extremely powerful.
Never should you have an instantiation of unit of work, repositories, … throughout your application code.
You’re not the first person using UnitOfWork and IOC. There are tons of examples around on how to do this. I advise you to dig into IOC first (and not only how, but also why you use it, and what the benefits are), whether it’s ninject or something else, that doesn’t matter. I understand IOC being hard at first, but it’s easy and useful once you master it.
Edit Example:
Below is an example on how I would implement this.
Please note, I do not use a separate unit of work, since my EF context is my unit of work.
The first issue you have is that you are not using interfaces; so my example is based on yours but I’ve added interfaces.
IMainContext interface:
public interface IMainContext
{
IDbSet<TEntity> Set<TEntity>() where TEntity : class;
DbEntityEntry<TEntity> Entry<TEntity>(TEntity entity) where TEntity : class;
int SaveChanges();
}
IMainContext implementation for Entity Framework
public class MainContext : DbContext, IMainContext
{
// your original MainContext code goes here
}
IGenericRepository
public interface IGenericRepository<TEntity> where TEntity : class
{
IEnumerable<TEntity> Get(
Expression<Func<TEntity, bool>> filter = null,
Func<IQueryable<TEntity>, IOrderedQueryable<TEntity>> orderBy = null,
string includeProperties = "");
TEntity GetByID(object id);
void Insert(TEntity entity);
void Delete(object id);
void Delete(TEntity entityToDelete);
void Update(TEntity entityToUpdate);
}
GenericRepository
public class GenericRepository<TEntity> : IGenericRepository<TEntity> where TEntity : class
{
private readonly IMainContext _context;
private readonly IDbSet<TEntity> _dbSet;
public GenericRepository(IMainContext context)
{
this._context = context;
this._dbSet = context.Set<TEntity>();
}
public virtual IEnumerable<TEntity> Get(
Expression<Func<TEntity, bool>> filter = null,
Func<IQueryable<TEntity>, IOrderedQueryable<TEntity>> orderBy = null,
string includeProperties = "")
{
IQueryable<TEntity> query = _dbSet;
if (filter != null)
{
query = query.Where(filter);
}
foreach (var includeProperty in includeProperties.Split
(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries))
{
query = query.Include(includeProperty);
}
if (orderBy != null)
{
return orderBy(query).ToList();
}
else
{
return query.ToList();
}
}
public virtual TEntity GetByID(object id)
{
return _dbSet.Find(id);
}
public virtual void Insert(TEntity entity)
{
_dbSet.Add(entity);
}
public virtual void Delete(object id)
{
TEntity entityToDelete = _dbSet.Find(id);
Delete(entityToDelete);
}
public virtual void Delete(TEntity entityToDelete)
{
if (_context.Entry(entityToDelete).State == EntityState.Detached)
{
_dbSet.Attach(entityToDelete);
}
_dbSet.Remove(entityToDelete);
}
public virtual void Update(TEntity entityToUpdate)
{
_dbSet.Attach(entityToUpdate);
_context.Entry(entityToUpdate).State = EntityState.Modified;
}
}
ICityRepository
public interface ICityRepository : IGenericRepository<City>
{
}
CityRepository
public class CityRepository : GenericRepository<City>, ICityRepository
{
public CityRepository(IMainContext context) : base(context)
{
}
}
// I Used country as a second object, I have no idea what object you have besides City.
// I wanted two objects to demonstrate UOW usage
public class Country
{
}
public interface ICountryRepository : IGenericRepository<Country>
{
}
public class CountryRepository : GenericRepository<Country>, ICountryRepository
{
public CountryRepository(IMainContext context)
: base(context)
{
}
}
Now our controller constructor looks like this:
private readonly IMainContext _context;
private readonly ICityRepository _cityRepository;
private readonly ICountryRepository _countryRepository;
public HomeController(IMainContext context, ICityRepository cityRepository, ICountryRepository countryRepository)
{
_context = context;
_cityRepository = cityRepository;
_countryRepository = countryRepository;
}
Now we can register our dependencies in MVC (global.asax)
public class MvcApplication : System.Web.HttpApplication
{
protected void Application_Start()
{
AreaRegistration.RegisterAllAreas();
WebApiConfig.Register(GlobalConfiguration.Configuration);
FilterConfig.RegisterGlobalFilters(GlobalFilters.Filters);
RouteConfig.RegisterRoutes(RouteTable.Routes);
BundleConfig.RegisterBundles(BundleTable.Bundles);
var ninjectKernel = new StandardKernel();
ConfigureDependencies(ninjectKernel);
DependencyResolver.SetResolver(new NinjectDependencyResolver(ninjectKernel));
}
// Dependencies should be configured in a separate project, not inside the web. This is for demo
private void ConfigureDependencies(StandardKernel ninjectKernel)
{
ninjectKernel.Bind<IMainContext>().To<MainContext>().InRequestScope();
ninjectKernel.Bind<ICityRepository>().To<CityRepository>();
ninjectKernel.Bind<ICountryRepository>().To<CountryRepository>();
}
}
This is basically how you do it. I would refer you to try to understand IOC, since I’m pretty sure you are missing the point.
Do I need ninject when implementing DAL with generic repository and unit of work?
That’s not the question. Inversion of control, repository and unit of work are patterns. Ninject is an IoC container – a tool that resolves dependencies and controls instantiation and object lifetime.
You need Ninject (excellent choice, but many other IoC containers are available) when poor man’s DI – manually injecting the dependencie, becomes combersome.
Whether your strategy for data access is a repository+UoW over ado.net, or just an entity-framework DbContext that your HomeController receives as a constructor argument, is of no relevance in the decision on whether or not you need to use Ninject/IoC.
I instatiate unit of work in controller is this ok to it like this?
private UnitOfWork unitOfWork = new UnitOfWork();
No. If you’re going to go DI, go DI all the way.
By newing up your UoW like this, you have tightly coupled HomeController with UnitOfWork: from that point on, everything in this class that happens with this unitOfWork object, is something unit tests cannot isolate.
With DI/IoC, using the new keyword anywhere other than:
- the composition root (as close as possible / at the application’s entry point).
- factory implementations, which might not even be needed with Ninject.
- when instantiating trivial BCL types that the IoC container doesn’t need to know about.
…is a synonym of a missed DI opportunity: any type that controls its dependencies increases coupling in your application – let go of that control, it’s Ninject’s job to worry about how objects are created. There’s a DI anti-pattern called “Control Freak” for a reason 😉
Furthermore, the only interface your UnitOfWork class implements, is IDisposable: the HomeController class, and all classes that have a dependency on the UnitOfWork type, are tightly coupled with the specific implementations in that UnitOfWork class.
Depend on abstractions, not implementations.
That, is the fundamental premise of Dependency Injection. So Instead of assigning unitOfWork to a new UnitOfWork();, I would have expected to see something like this:
private readonly IUnitOfWork _unitOfWork;
public HomeController(IUnitOfWork unitOfWork)
{
_unitOfWork = unitOfWork; // or this.unitOfWork = unitOfWork;
}
Yeah, ok. Now what?
This creates a problem (how will ASP.NET MVC know how to instantiate the controller now?), that you effortlessly solve with Ninject in Global.asax.cs, in the Application_Start handler:
ControllerBuilder.Current.SetControllerFactory(new NinjectControllerFactory());
The Global.asax.cs file is your entry point – the deal spot for your composition root.
Make sure your read about the Ninject.Web.Mvc3 namespace on GitHub; if you’re referencing the binaries, you should go like this:
Create a new MVC application and add references to Ninject, Ninject.Web.Common and Ninject.Web.Mvc3 from the Github download. Then change the global.asax to derive from NinjectHttpApplication instead of HttpApplication and override CreateKernel to create a kernel and load all modules that you need in your application.
The documentation also says how to set it up if you’re using Ninject through a NuGet package.
Don’t go reinventing the wheel here, like Mark Seemann puts it in Dependency Injection in .NET:
[…] Some DI CONTAINERS also have “official” ASP.NET MVC integration.
This is exactly what we’re talking about: Ninject’s way of integrating with ASP.NET MVC.
UnitOfWork
As I mentioned above, you’re missing an IUnitOfWork abstraction. I’d do something like this:
public interface IUnitOfWork
{
void SubmitChanges();
IDbSet<TEntity> Set();
}
But Mat, you’re leaking EF through your abstraction! I am. And if depending on a DbContext didn’t mean unit tests actually hit the database, I’d inject a DbContext directly.
The idea isn’t to push EF under the carpet and pretend it’s not there: like Frederick P. mentioned in his answer, exposing IQueryable, even as a parameter, is leaking Linq-to-Entities – you’re using entity-framework, why fight it?
A DbContext is a UoW – and a DbSet is a repository. If UoW+repository are abstractions, then what are you trying to abstract exactly? Abstractions of abstractions are overkill IMO, embrace EF. Your life will be much simpler.
public class UnitOfWork : DbContext, IUnitOfWork
{
public IDbSet<Country> Countries { get; set; }
public IDbSet<City> Cities { get; set; }
public IDbSet<TEntity> Set<TEntity>()
{
return base.Set<TEntity>();
}
protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Configurations.Add(new CountryMappings());
modelBuilder.Configurations.Add(new CityMappings());
}
}
Why expose
IDbSet<TEntity> Set()?Because exposing
DbSet<TEntity>would be coupling our interface with that specific implementation ofIDbSet<TEntity>. By exposing an interface, you’re still free to return anything that implements it.The alternative is to expose an
IDbSet<Country>, and then anIDbSet<City>, and then who knows what the future holds for your data model? Interfaces shouldn’t be designed to change, because modifying an interface is a breaking change.Of course, another alternative would be to expose
IRepository<Country>instead, but then we’d just be coating EF with useless interfaces, and considerably complicate everything.
This means your HomeController could look like this:
public class HomeController : Controller
{
// private readonly IUnitOfWork unitOfWork;
private readonly IUnitOfWork _unitOfWork;
public HomeController(IUnitOfWork unitOfWork)
{
// this.unitOfWork = unitOfWork;
_unitOfWork = unitOfWork;
}
public ActionResult Index()
{
var cities = _unitOfWork.Set<City>().ToList();
return View(cities);
}
public override void Dispose()
{
if (_unitOfWork is IDisposable)
{
((IDisposable)_unitOfWork).Dispose();
}
base.Dispose();
}
}
Is this EF? Your controller is depending on an IUnitOfWork. _unitOfWork could just as well be a mock implementation that a mocking framework such as Moq would have configured to return an InMemoryDbSet<City> whenever the Set<City>() method was called:
var mockCities = new InMemoryDbSet<City>(){ new City(), new City()}; var mockUoW = new Mock<IUnitOfWork>(); mockUoW.Setup(uow => uow.Set<City>()).Returns(mockCities); var controller = new HomeController((IUnitOfWork)mockUoW); var view = controller.Index(); // ...Largely inspired from https://stackoverflow.com/a/16214880/1188513
Is there really a need for any more abstraction than that? kiss.
Your code has marker interfaces:
public interface ICountryRepository : IGenericRepository<Country>
{
}
That should raise a flag; interfaces aren’t just metadata – marker interfaces are seldom legitimate in a SOLID design.
Addentum: Conventions over Configuration
In your RegisterServices implementation, you don’t even need to explicitly Bind every single dependency. Because your default IUnitOfWork implementation is called ..UnitOfWork (right?), you can use Ninject.Extensions.Conventions and BindDefaultInterface on all classes in your DAL; when the composition root composes your application and resolves a HomeController, IUnitOfWork will resolve to UnitOfWork, because with BindDefaultInterface, IFoo binds to Foo.
If you’re not using Ninject.Extensions.Conventions, you’re really missing out.