3
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
6
\$\begingroup\$

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)
    {
        _dbSet.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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ There's a typo _deset in the add method. I'd edit, but it'd get rejected :) \$\endgroup\$ – Carrie Kendall Jun 19 '17 at 21:50
  • \$\begingroup\$ I like the 'generic' approach, but at most I agreed with your 'Finally' conclusion - the EF is already implementing Repo pattern, so adding next layer only for 'pattern' is not 'good practice' - unless you have a specific reason to do it and a new layer will give you some other benefits. \$\endgroup\$ – Lukasz Makowej Jun 20 '17 at 9:35
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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

The entity framework already has a method for loading related data: See this link section 'Include related data'. Older versions of the entity framework have similar methods.

That would turn your GetAll method into a one liner ;)

_context.Organizations.Include(o => o.Address);

As Gert Arnold already mentioned, think about the necessity of the repository class. The DbContext already is something like a flexibly repository... IMHO it is not necessarily required to add another abstraction (except if the abstraction really adds value).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The DbContext already is something like a flexible repository... IMHO it is not necessarily required to add another abstraction (except if the abstraction really adds value). - agreed! \$\endgroup\$ – Lukasz Makowej Jun 20 '17 at 9:36

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.