Saving a sales order - too many repositories?

Question

I am wondering if I have overdone it with repositories in the following code, which is to save a sales order.

I understand that the purpose of a repository is to decouple the domain layer from the persistence layer, but this code seems to be replicating what EF does anyway.

public SalesOrder FindSalesOrder(int id)
{
    using (var uow = new UnitOfWork<LogContext>())
    {
        using (var repository = new SalesOrderRepository(uow))
        {
            var SalesOrder = repository.Find(s => s.Id == id, s => s.Site, s => s.User);
            if (SalesOrder != null)
            {
                using (var repository2 = new OrderLineRepository(uow))
                {
                    var OrderLines = repository2.GetList(o => o.SalesOrderId == id);
                    foreach (var OrderLine in OrderLines)
                    {
                        OrderLine.SalesOrder = SalesOrder;
                        SalesOrder.OrderLines.Add(OrderLine);

                        using (var repository3 = new LineDetailRepository(uow))
                        {
                            var LineDetails = repository3.GetList(v => v.OrderLineId == OrderLine.Id, v => v.PropertyName);
                            foreach(var LineDetail in LineDetails)
                            {
                                LineDetail.OrderLine = OrderLine;
                                OrderLine.LineDetails.Add(LineDetail);
                            }
                        }
                    }
                }
            }

            return SalesOrder;
        }
    }
}

[Update] I had a chat to my co-developer and think that perhaps the code is OK.

Our reasoning is that EF itself is rather complicated so we want to have a wrapper around what we allow the UI layer to use.

We did realize that we don't need the lineDetail repository as there are no instances where we need to access the lineDetails separate from the orderLines. That means we could make the orderLines return containing the lineDetails pre-populated.

However that would then mean some of our repository's Find methods return entities with children and some don't. Thus I am thinking maybe we need a FindGraph method in our repositories which will return the full graph.

However now I wonder whether the FindSalesOrder method should be in the SalesOrder repository itself instead of the UI layer?

Answer 1

I find it interesting that you want to decouple layers, but that the method you have here is tightly coupled with UnitOfWork<LogContext> (and thus with LogContext as well), OrderLineRepository and OrderDetailRepository.

To answer the question directly, yes, it's way overdone.

I'd put a method like SalesOrder FindSalesOrder(int id) in a service layer:

public class SalesOrderService
{
    private readonly IUnitOfWork _uow;

    public SalesOrderService(IUnitOfWork uow)
    {
        _uow = uow;
    }

    public SalesOrder FindSalesOrder(int id)
    {
        return _uow.Set<SalesOrder>() // get repository
                   .Where(order => order.Id == id) // filter by id
                   .Include(order => order.OrderLines) // eager-load details
                   .ToList(); // materialize
    }
}

What's IUnitOfWork, you'll ask? Just an abstraction that looks something like this:

internal interface IUnitOfWork
{
    DbSet<TEntity> Set<TEntity>();
    void Commit();
}

The EF context itself implements it:

public class MyDbContext : DbContext, IUnitOfWork
{
    public IDbSet<SalesOrder> Orders { get; set; }
    
    public void Commit()
    {
        SaveChanges();
    }
}

Entity Framework's DbContext is a unit-of-work implementation, and IDbSet<T> is a repository.

Constructor-injecting an IUnitOfWork in your service class effectively decouples your service class from EF, at least good enough for unit testing (you can mock what Set<T>() returns)... "swapping the ORM" will require lots of work anyway, and is highly hypothetical - the cost vs benefits ratio doesn't add up in favor of abstracting EF with repositories.

---

Your Code

There are a number of itchy spots with the code you've provided:

• Casing is annoyingly inconsistent for local variables. Should be camelCase, always: var LineDetails becomes var lineDetails, for example.
• using blocks should be stacked whenever possible, to reduce unnecessary nesting:

using (var uow = new UnitOfWork<LogContext>())
{
    using (var repository = new SalesOrderRepository(uow))
    {
        var SalesOrder = repository.Find(s => s.Id == id, s => s.Site, s => s.User);
        if (SalesOrder != null)
        {

Becomes:

using (var uow = new UnitOfWork<LogContext>())
using (var repository = new SalesOrderRepository(uow))
{
    var SalesOrder = repository.Find(s => s.Id == id, s => s.Site, s => s.User);
    if (SalesOrder != null)
    {