ASP.NET core proper way to work with the repository pattern

Question

I am currently working on my first ASP.NET Core MVC project with a repository pattern.

I have gotten it to work but I wonder if there is a better way to solve this.

In this case I have an Organization model and an Address model, with OrganizationData and AddressData for the repositories.

What I am wondering about the most is how I handled the relationship between organization and address.

Organization model

using System;
using System.ComponentModel.DataAnnotations;

namespace Verbonding.Models
{
    public class Organization
    {
        public int Id { get; set; }    
        [Required, MaxLength(80)]
        [DataType(DataType.Text)]
        [Display(Name = "Organization Name")]
        public string Name { get; set; }    
        [DataType(DataType.EmailAddress)]
        public string Email { get; set; }    
        [DataType(DataType.Url)]
        public string Website { get; set; }    
        public int AddressId { get; set; }    
        public bool IsActive { get; set; }
        public bool IsBlocked { get; set; }
        public DateTime DateCreated { get; set; }
        public DateTime DateUpdated { get; set; }    
        public virtual Address Address { get; set; }

        public Organization()
        {
            IsActive = true;
            IsBlocked = false;
            DateCreated = DateTime.Now;
            DateUpdated = DateTime.Now;
        }
    }
}

Address model

using System.ComponentModel.DataAnnotations;

namespace Verbonding.Models
{
    public class Address : IAddress
    {
        public int Id { get; set; }
        [Required]
        public string Street { get; set; }
        [Required]
        public string HouseNr { get; set; }
        [Required]
        public string PostalCode { get; set; }
        [Required]
        public string City { get; set; }
        public int? CountryId { get; set; }
        public virtual Country Country { get; set; }
    }
}

OrganizationData repository

What I am wondering about here is if the GetAll() method could me a bit nicer.

using System.Collections.Generic;
using System.Linq;
using Verbonding.Data;
using Verbonding.Models;

namespace Verbonding.Services
{
    public class OrganizationData : IOrganizationData
    {
        private ApplicationDbContext _context;

        public OrganizationData(ApplicationDbContext context)
        {
            _context = context;
        }

        public Organization Add(Organization organization)
        {
            _context.Add(organization);
            return organization;
        }

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

        public void Delete(int id)
        {
            Organization organization = Get(id);
            _context.Remove(organization);
        }

        public Organization Get(int id)
        {
            return _context.Organizations.FirstOrDefault(o => o.Id == id);
        }

        public IEnumerable<Organization> GetAll()
        {
            IEnumerable<Organization> organizations = _context.Organizations;

            var addressData = new AddressData(_context);

            foreach (Organization o in organizations)
            {
                o.Address = addressData.Get(o.AddressId);
            }
            return organizations;   
        }
    }
}

AddressData repository

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Verbonding.Data;
using Verbonding.Models;
namespace Verbonding.Services
{
    public class AddressData : IAddressData
    {
        private ApplicationDbContext _context;
        public AddressData(ApplicationDbContext context)
        {
            _context = context;
        }
        public Address Add(Address address)
        {
            _context.Add(address);
            return address;
        }
        public void Commit()
        {
            _context.SaveChanges();
        }
        public void Delete(int id)
        {
            Address address = Get(id);
            _context.Addresses.Remove(address);
        }
        public Address Get(int id)
        {
            return _context.Addresses.FirstOrDefault(a => a.Id == id);
        }
        public IEnumerable<Address> GetAll()
        {
            return _context.Addresses;
        }
    }
}

OrganizationViewModel

namespace Verbonding.Models.OrganizationViewModels
{
    public class OrganizationViewModel
    {
        public string Name { get; set; }
        public string Email { get; set; }
        public string Website { get; set; }
        public string Street { get; set; }
        public string HouseNr { get; set; }
        public string PostalCode { get; set; }
        public string City { get; set; }
        public Country Country { get; set; }
    }
}

IndexViewModel

using System.Collections.Generic;

namespace Verbonding.Models.OrganizationViewModels
{
    public class IndexViewModel
    {
        public IEnumerable<Organization> Organizations{ get; set; }
    }
}

OrganizationsController

I have left some methods out here that have not been implemented yet.

In the POST Create() method I am mapping the values manually to the model, and saving them to the repository.
Is there a better way to do this, or is this fine the way it is.

using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc;
using Verbonding.Models;
using Verbonding.Services;
using Verbonding.Models.OrganizationViewModels;

namespace Verbonding.Controllers
{
    public class OrganizationsController : Controller
    {
        private IOrganizationData _organizationData;
        private IAddressData _addressData;

        public OrganizationsController(IOrganizationData organizationData,
                                        IAddressData addressData)
        {
            _organizationData = organizationData;
            _addressData = addressData;
        }

        // GET: Organizations
        public IActionResult Index()
        {
            var model = new IndexViewModel();
            model.Organizations = _organizationData.GetAll();
            return View(model);
        }

        // GET: Organizations/Create
        public IActionResult Create()
        {
            return View();
        }

        // POST: Organizations/Create
        // To protect from overposting attacks, please enable the specific properties you want to bind to, for 
        // more details see http://go.microsoft.com/fwlink/?LinkId=317598.
        [HttpPost]
        [ValidateAntiForgeryToken]
        public IActionResult Create(OrganizationViewModel organization)
        {
            if (ModelState.IsValid)
            {
                var newOrganization = new Organization();
                var newAddress = new Address();

                newOrganization.Name = organization.Name;
                newOrganization.Email = organization.Email;
                newOrganization.Website = organization.Website;

                _organizationData.Add(newOrganization);

                newAddress.Street = organization.Street;
                newAddress.HouseNr = organization.HouseNr;
                newAddress.PostalCode = organization.PostalCode;
                newAddress.City = organization.City;
                newAddress.Country = organization.Country;
                _addressData.Add(newAddress);

                newOrganization.Address = newAddress;

                _organizationData.Commit();

                return RedirectToAction("Index");
            }
            return View(organization);
        }
    }
}

Thanks

Paweł Łukasik · Answer 1 · 2016-11-27 15:10:24Z

You could extract this logic

var newOrganization = new Organization();
var newAddress = new Address();

newOrganization.Name = organization.Name;
newOrganization.Email = organization.Email;
newOrganization.Website = organization.Website;

_organizationData.Add(newOrganization);

newAddress.Street = organization.Street;
newAddress.HouseNr = organization.HouseNr;
newAddress.PostalCode = organization.PostalCode;
newAddress.City = organization.City;
newAddress.Country = organization.Country;
_addressData.Add(newAddress);

newOrganization.Address = newAddress;

_organizationData.Commit();

to a separate thing (i.e. Command). With that you could rewrite your action in controller in such way

[HttpPost]
[ValidateAntiForgeryToken]
public IActionResult Create(OrganizationViewModel organization)
{
    if (ModelState.IsValid)
    {
       var insertToDb = new AddOrganizationCommand(organization, _organizationData, _addressData);
       addOrganization.Execute();
       return RedirectToAction("Index");
    }
    return View(organization);
}

this way you separate your logic from MVC (thin controllers). And by using the interfaces in the command you can easily test your code by injecting fake/mocked repositories.

For mapping the entities you could use AutoMapper.

Gert Arnold · Answer 2 · 2016-12-02 11:23:16Z

Better? Probably...

All your repositories look almost identical, only the served type is different. Therefore, you may as well turn to generic repository. In its elementary form, adopting your methods, it looks like:

public class GenericRepository<TEntity> where TEntity : class
{
    private ApplicationDbContext _context;
    private DbSet<TEntity> _dbSet;

    public GenericRepository(ApplicationDbContext context)
    {
        this._context = context;
        this._dbSet = context.Set<TEntity>();
    }

    public TEntity Add(TEntity entity)
    {
        _deSet.Add(entity);
        return entity;
    }

    public void Delete(int id)
    {
        TEntity entity = Get(id);
        _dbSet.Remove(entity);
    }

    public TEntity Get(int id)
    {
        return _dbSet.Find(id);
    }

    public IQueryable<TEntity> GetAll()
    {
        return _dbSet;
    }
}

Where's Commit?

No, I didn't forget the Commit method. The fact is, repositories shouldn't commit. Maybe this is surprising, but if you think about transaction management it becomes obvious. There may be business transactions in which several repositories are involved. When each repo has the potential to save all changes, it may be very hard to figure out which one is able to save at the right moment. That's the reason why generic repos always come with a Unit-of-Work pattern. This is all explained well enough in the link above.

You can see this problem lurking in your code. At the end you have

_addressData.Add(newAddress);
newOrganization.Address = newAddress;
_organizationData.Commit();

So you decide that _organizationData should save the changes. It might as well have been _addressData. Using either one, it's not clear from the code that the other data are saved as well. If you surround the code by a UoW, it's clear that the UoW commits the changes transactionally.

I guess this is also the piece of code that raised your question

What I am wondering about the most is how I handled the relationship between organization and address.

Well, the lines ...

_addressData.Add(newAddress);
newOrganization.Address = newAddress;

... are redundant. If you set newOrganization.Address and then ...

_organizationData.Add(newOrganization);

... the address is added as well.

GetAll

What I am wondering about here is if the GetAll() method could me a bit nicer.

Note that GetAll doesn't return IEnumerable but IQueryable. This opens the opportunity to compose a LINQ statement involving multiple repositories and still translate the whole statement into one SQL statement.

For example, if you need an occasional join (because there is no navigation property), it would look like:

repoA.GetAll().Where(a => a.Date > someDate)
.Join(repoB.GetAll(), a => a.Code, b => b.Code)
.Select( ....

If GetAll returns IEnumerable you'll see that the as and bs (all bs) will be fetched into memory by two queries. Also, the Select is executed in memory, not translated into SQL. With IQueryable, the whole statement is translated into SQL, making it far more efficient. (Assuming, of course, that both repos receive the same context instance).

Finally

There's always much discussion about the use of generic repo/UoW on top of EF's DbSet/DbContext that implement the same patterns. I wouldn't use them just because it's a "good pattern". In most cases they're only a thin wrapper around the EF objects. Maybe you have to reevaluate this.

t3chb0t · Answer 3 · 2016-11-28 08:25:39Z

The reason why we use a repository is the encapsulation of the data(base) access. You don't only want make accessing your data easier for the user but you also want to have all the queries in one place so that you can test and keep an eye on them.

This means that allowing or rather forcing the user to know how you store it breaks the pattern.

public OrganizationData(ApplicationDbContext context)

He needs to pass it a DbContext. It's not like you would change it frequently. Most probably never because if you change it, it means the data model has changed and it wont't work anyway. The only thing you might want to is to change the connection string to test it against a different database.

I find a repository like this one is just a different DbContext around the EF DbContext. You haven't gained anything. You still have the Commit, Save etc. with the additional work by wrapping all entities in repositories.

In most scenarios you just need a bunch of simple queries to get some results or add a few objects to the database. Building a CRUD repository for each entity type is IMO a total overkill. Ask yourself if you really need it? Don't just use it because a repository pattern is so cool. It isn't if you just create it for the sake of having implemented a pattern. You put a lot of work in it and didn't win anything.

You'd probably be much better off with simply encapsulating the queries with few methods provided by a repository that requires nothing but a connection string.

It shouldn't be a problem passing the ApplicationDbContext since this could be actually done by an IoC container. I agree with the fact that we could use for CRUD the ApplicationDbContext itself. — Adrian Iftode, Nov 30 '16 at 20:54

asked	2 months ago
viewed	170 times
active	1 month ago

current community

your communities

more stack exchange communities

ASP.NET core proper way to work with the repository pattern

Organization model

Address model

OrganizationData repository

AddressData repository

OrganizationViewModel

IndexViewModel

OrganizationsController

3 Answers 3

Better? Probably...

Where's Commit?

GetAll

Finally

Your Answer

Not the answer you're looking for? Browse other questions tagged c# entity-framework asp.net-mvc or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

ASP.NET core proper way to work with the repository pattern

Organization model

Address model

OrganizationData repository

AddressData repository

OrganizationViewModel

IndexViewModel

OrganizationsController

3 Answers 3

Better? Probably...

Where's Commit?

GetAll

Finally

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c# entity-framework asp.net-mvc or ask your own question.

Related

Hot Network Questions