4
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ Have you tried just returning the Sales Order? Typically if you have FK on your records EF will go off and fetch the values for you when you reference them later on i.e. SalesOrder.OrderLines will go off and fetch all the order lines belonging to the SalesOrder by Id \$\endgroup\$ – dreza May 7 '14 at 2:27
  • \$\begingroup\$ I know, that's why i am wondering what use the above design is. I thought repositories were worth adopting but now i am having a hard time seeing their purpose. \$\endgroup\$ – kirsten g May 7 '14 at 2:30
  • 3
    \$\begingroup\$ They may be usefull for initial object querying when you don't have the parent object avaialble. ie. if you wanted to get all OrderLines by SalesId. Then a repository could do that. Otherwise I tend to leave EF as the repository and use a Service layer instead. \$\endgroup\$ – dreza May 7 '14 at 2:35
  • \$\begingroup\$ Why you are doning that?! SalesOrder has a list of OrderLines this is 1..n relationship just use lazy or eager loading to load them and not use the id !? same here between Orderline and LineDetail \$\endgroup\$ – Bassam Alugili May 7 '14 at 9:35
  • 1
    \$\begingroup\$ I'd be tempted to leave EF as is for this, however implementing your own repository pattern to sit on top of EF is a good idea if you plan on swapping EF out for something else, it can also help with testability. \$\endgroup\$ – Joe May 7 '14 at 11:15
2
\$\begingroup\$

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)
    {
\$\endgroup\$

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.