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

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 abou

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 where TEntity : class
{
    private ApplicationDbContext _context;
    private DbSet _dbSet;

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

    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 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.

StackExchange Code Review Q#148244, answer score: 6