1
\$\begingroup\$

I have these queries written in Entity Framework where I try to get:

  • Name of the customer
  • Most Recent Invoice Ref
  • Most Recent Invoice Amount (£)
  • Number of outstanding invoices (#)
  • Total of all outstanding invoices (£)

I use these two tables:

CREATE TABLE Customers
(
    CustomerId INT,
    Name NVARCHAR(100),
    Address1 NVARCHAR(100),
    Address2 NVARCHAR(100),
    Postcode NVARCHAR(100),
    Telephone NVARCHAR(15),
    CONSTRAINT Customers_PK PRIMARY KEY (CustomerId)
)

CREATE TABLE Invoices
(
    InvoiceId INT,
    CustomerId INT, -- FK
    Ref NVARCHAR(10),
    InvoiceDate DATETIME,
    IsPaid BIT,

    Value DECIMAL,
    CONSTRAINT Invoices_PK PRIMARY KEY (InvoiceId)
)

And the queries I make are:

public PaginatedList<CustomerListEntity> GetCustomers(int pageIndex, int pageSize, out int totalRecords)
        {
            var customers = _customerRepository.AllIncluding(x => x.Invoices).OrderBy(x => x.CustomerId).Select(x => new
              CustomerListEntity {
                Name = x.Name,
                CustomerID=x.CustomerId,
                RecentInvoiceRef = x.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault() != null ? x.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault().Ref : string.Empty,
                RecentInvoiceAmount = x.Invoices.OrderByDescending(t=>t.InvoiceDate).FirstOrDefault()!=null? x.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault().Value:null,
                UnpaidInvoicesNumber=x.Invoices.Where(t=>t.IsPaid==false).Count(),
                UnpaidInvoicesTotalAmount = x.Invoices.Where(t => t.IsPaid == false).Sum(k=>k.Value)
            });
            totalRecords = customers.Count();
            return customers.ToPaginatedList(pageIndex, pageSize);
        }

Here is the implementation of AllINcluding:

public virtual IQueryable<T> AllIncluding(params Expression<Func<T, object>>[] includeProperties)
    {
        IQueryable<T> query = _entitiesContext.Set<T>();

        foreach (var includeProperty in includeProperties)
        {
            query = query.Include(includeProperty);
        }
        return query;
    }
\$\endgroup\$
1
\$\begingroup\$

First off, the Include is ignored because you project the result to a new result (Select(x => new{ ... }). So you may as well remove it.

Then, there is some room for improvement by eliminating the repetitive parts. It's much easier to do this in query syntax:

var customers = from cst in _customerRepository.All() // Assuming this method exists
    let lastInvoice = cst.Invoices.OrderByDescending(t => t.InvoiceDate).FirstOrDefault()
    let upaidInvoices = cst.Invoices.Where(t => !t.IsPaid)
    select new
      CustomerListEntity {
        Name = cst.Name,
        CustomerID = cst.CustomerId,
        RecentInvoiceRef = lastInvoice.Ref ?? string.Empty,
        RecentInvoiceAmount = lastInvoice.Value,
        UnpaidInvoicesNumber = upaidInvoices.Count(),
        UnpaidInvoicesTotalAmount = upaidInvoices.Sum(k => k.Value)
    });

The let keyword defines a local variable that can be reused in the LINQ statement. As you see, this greatly improves the readability of the code.

It also slightly improves the generated SQL query, because Ref and Value will now be retrieved in one subquery instead of two. Unfortunately, this doesn't apply to Count and Sum, because aggregates require separate subqueries.

Also, note that the null checks are removed. The LINQ statement is translated into SQL and executed in the database. SQL doesn't have this null reference concept.

One last point is that there is broad consensus on the additional repository layer being totally redundant. Consider removing and query directly on the context.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ While this is a case where I have to (begrudgingly) agree that LINQ's query syntax is better, if OP is working in a codebase that only uses method syntax, I'd suggest sticking to conformity unless there is significant impact (which there IMO isn't in this case). \$\endgroup\$ – Flater Nov 6 '19 at 11:45
  • \$\begingroup\$ "there is broad consensus on the additional repository layer being totally redundant" I very much disagree. Unless you're advocating the leaking of EF outside of the DAL (which is a massive red flag that I can't tackle in a mere comment), your DAL will always need some way to not leak EF to consumers. Which pattern/interface/construct you use for that separation is up for debate, but the "repository" name tends to persist regardless of pattern/interface/construct due to this separation's functional purpose of providing access to the data store. \$\endgroup\$ – Flater Nov 6 '19 at 11:50
  • \$\begingroup\$ If EF were to step away from inheriting from its context and instead allow for a composition-over-inheritance approach, then your custom db context would be able to be exposed publically without needing to leak EF itself. But as far as I'm aware that is not the case, your custom db context currently needs to inherit EF's DbContext which leads to requiring a reference to the EF library in order to work with your custom db context. \$\endgroup\$ – Flater Nov 6 '19 at 11:52
  • \$\begingroup\$ @Flater If really necessary this can be converted into method syntax, throwing away the readability (big time) but retaining the slight SQL improvement. About leaky abstractions: they are inevitable in any data layer based on IQueryable. As for composition-over-inheritance, I can only agree, it's a far better pattern. How to tell EF though? \$\endgroup\$ – Gert Arnold Nov 6 '19 at 12:03
  • \$\begingroup\$ You're right about IQueryables always being a leaky abstraction, hence the good practice suggestion of not leaking IQueryables. OP is indeed leaking IQueryables at the moment, but that's still less of a transgression than leaking EF is. (EF leak = EF library dependency in consumers; IQueryable leak = needing to know which expressions EF cannot convert to SQL for you, but no EF library dependency in consumers) It's not an improvement to throw the baby out with the bathwater and start leaking EF altogether. \$\endgroup\$ – Flater Nov 6 '19 at 12:06

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.