13
\$\begingroup\$

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);
        }

    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Are there any specific areas you want more detail in? \$\endgroup\$ – Ben Aaronson Jul 18 '14 at 20:41
  • \$\begingroup\$ I just want to know how to use context in repository implementation based on @Frederik P. answer. In MainContext I added all my dbsets but don't see them in concrete repositories. So I can't make custom queries. \$\endgroup\$ – 1110 Jul 18 '14 at 20:46
  • \$\begingroup\$ @BenAaronson I'd be curious to hear [/read] your thoughts about my answer :) \$\endgroup\$ – Mathieu Guindon Jul 19 '14 at 17:52
8
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I follow a few examples and came up with code above. Before I used IOC but with repository never with generic repository and unit of work. So I should create IUnitOfWork and bind it to UnitOfWork using ninject and add it in controller constructor, right? But example that I follow used generic repository and add repository instances in UnitOfWork as I show in question. So I can inject UnitOfWork in my WEB project but my UOW already has repositories inside. This is what confuse me. Do I need only to inject UOW or all repositories but then I don't see why I need repos inside UOW? \$\endgroup\$ – 1110 Jul 16 '14 at 8:47
  • 1
    \$\begingroup\$ I always use a Generic Repository, but my Unit Of Work is only used to SaveChanges or RollbackChanges. DbContext of Entity Framework is a UnitOfWork pattern in itself. There is no special need to create a seperate one. If you do, there is still no need to have all the repositories in your Unit of Work. But if you realy want that, you need to inject your repositories in the constructor of your unit of work. But that's not the best pattern to follow. \$\endgroup\$ – Frederik P. Jul 16 '14 at 8:52
  • \$\begingroup\$ Can you show me how to redesign my solution (remove repositories from UOW) or give me some good tutorial on how to implement all in the right way? Sorry now I read comment again. Do you want to say that I don't need UnitOfWork? So if I don't need it I need only repositories right? \$\endgroup\$ – 1110 Jul 16 '14 at 9:00
  • \$\begingroup\$ Updated my answer with an example \$\endgroup\$ – Frederik P. Jul 16 '14 at 9:39
  • \$\begingroup\$ Thanks. Just a little sub question. If I wan't to use for example CityRepository from CountryRepository (I don't know if ever need me but just want to know) I should also create constructor inside repository that accept repositories as parameters and inject them via ninject? \$\endgroup\$ – 1110 Jul 16 '14 at 10:01
8
+50
\$\begingroup\$

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 , or just an 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.

http://github.com/ninject/ninject.web.mvc/wiki/Setting-up-an-MVC3-application#using-binaries-from-github

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 , 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 of IDbSet<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 an IDbSet<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? .


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.

\$\endgroup\$
  • 1
    \$\begingroup\$ And if you want to keep EF references in your DAL, then instead of the UoW you inject the controller wih some service that depends on an IUnitOfWork and encapsulates your data operations. You may also want to send a ViewModel to the view, rather than entity types (POCO's). \$\endgroup\$ – Mathieu Guindon Jul 19 '14 at 4:52
  • \$\begingroup\$ "The idea isn't to push EF under the carpet and pretend it's not there: like Frederick P. mentioned in his answer". You have a great answer, but statements like that makes me wonder if you realy get the whole point. I've been on projects where we eventually had to swap out EF and use NHibernate or even ADO.NET. I can assure you, if you didn't hide EF onder the carpet you'll be sorry. \$\endgroup\$ – Frederik P. Jul 21 '14 at 10:47
  • \$\begingroup\$ None of my applications should ever be dependend on thirth party frameworks: NLogger, EF, Automapper, ... They will all be hidden under the carpet to make sure maintanace is easy. That's where it's all about, abstractions enables easily swapping layers (and even thirth party technologies). So if you CAN remove the thight coupling, why not do so? It doesnt take more time and doesnt introduce a greater complexity. \$\endgroup\$ – Frederik P. Jul 21 '14 at 10:50
  • \$\begingroup\$ "If you're not using Ninject.Extensions.Conventions, you're really missing out." That's a decision. Personally I prefer to do it manually, but there's alot of arguing I have about this. I think scanning it removes the power of IOC in some ways. I want to easily swap out implementations in the IOC. If tomorrow the ICityRepository needs to use the OracleCityRepository from a different assembly, I want to be able to easily swap out. By scanning the assembly and binding using conventions you're actually realy missing out. And I want to maintain my own dependencies, not auto resolve them. \$\endgroup\$ – Frederik P. Jul 21 '14 at 10:56
  • 2
    \$\begingroup\$ Nevermind bout the repo's, generaly you are right. But this doesnt mean: "Just use EF wherever you want". You still gotta be smart where you use the context. And that's the bad part, people start using it everywhere... But you've convinced me, kudo's for that :-) Upvoted, but I hope people be aware about EF entities shouldn't be used on the presentation side. I've seen EF Entities with MVC annotations, that realy hurt the eyes. \$\endgroup\$ – Frederik P. Jul 21 '14 at 11:22

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.