Optimize LINQ query inside a loop

Question

I have a piece of code that ensures that a customer's addresses are in sync with modifications made in the UI:

var customerAddresses = customer.CustomerAddresses.Select(x => x.Address);

// add address to customer if it does not already exist
foreach (var addressModel in model.Addresses)
{
    // make sure an address matches all properties
    Func<Address, bool> addressFilter = x => x.Id == addressModel.Id &&
             x.Street.Equals(addressModel.Street, StringComparison.OrdinalIgnoreCase) &&
             x.City.Equals(addressModel.City, StringComparison.OrdinalIgnoreCase) &&
             x.Province.Equals(addressModel.Province, StringComparison.OrdinalIgnoreCase) &&
             x.PostalCode.Equals(addressModel.PostalCode, StringComparison.OrdinalIgnoreCase);

    // check if customer already has this address
    if (!customerAddresses.Any(addressFilter))
    {
        // check if address already exists in database
        var address = this.DbContext.Addresses.SingleOrDefault(addressFilter);

        // add address if it does not exist
        if (address == null)
        {
            address = this.DbContext.Addresses.Add(new Address
            {
                Street = addressModel.Street,
                City = addressModel.City,
                Province = addressModel.Province,
                PostalCode = addressModel.PostalCode
            });
        }
    }

    this.DbContext.CustomerAddresses.Add(new InsuredAddress
    {
        Customer = customer,
        Address = address,
        IsPreferred = addressModel.IsPreferred
    });             
}

I am however concerned that the Func<Address, bool> addressFilter is created every time inside the loop. Is there a way to create filter in such a way that it accepts parameters and does not need to be recreated each time?

Ah, that makes more sense. I'd probably just add another parameter to the delegate for the loop variable, then. — Magus
– Magus, Commented Mar 18, 2014 at 14:55
Just wondering, can't you just have a your address class implement a IComparable / IEquatable interface and do the comparison that way, maybe? — John Bustos
– John Bustos, Commented Mar 18, 2014 at 14:56

Servy · Accepted Answer · 2014-03-18 15:04:21Z

2

It's easier to see the implications of this once you understand how anonymous methods are dealt with by the compiler.

The compiler is going to create some new time, with a arbitrary name. It will give that type an instance method with some other arbitrary name. The body of that method will be effectively the body of this anonymous method.

There will be an instance field for each closed over variable.

A new instance of this type will be created within the method, and the closed over variables will be replaced with access to the field of this closure class. The call to the anonymous method will be replaced with a call to the method in this new class.

Thus you should be able to see, based on this transformation, that the anonymous method is only ever compiled exactly once, regardless of its scope of its definition in this other method.

Having said all of that, you really shouldn't structure your program this way for entirely unrelated reasons. First off, you're defining a Func rather than an Expression<Func>, so the filter won't be able to be translated into a query executed on the database, rather you're pulling down the entire Addresses table twice (once when you call Any, and once when you call SingleOrDefault, for every single address in your model. That's really bad. I mean at the very, very least, you should be using an Expression to define the predicate so that the filtering can be done on the database side of things, and omitting the call to Any so that you're only doing one query per loop, but honestly, you shouldn't be performing multiple queries at all. What you should be doing is joining the two tables so that you get all of the information for the whole thing in one big query, rather than executing any queries in a loop.

edited Mar 18, 2014 at 15:04

answered Mar 18, 2014 at 14:59

Servy

204k27 gold badges347 silver badges470 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Alfero Chingono Over a year ago

You make very interesting points here. I honestly thought: (1) A Func also gets translated, and (2) Any was a good idea.

Alfero Chingono Over a year ago

I like your suggestion about the JOIN, but I can't wrap my head around how to do that. Care to provide an example? or link?

Servy Over a year ago

That's getting too offtopic to go into more detail that I already have. You can find lots of information on how to join tables through Google though; there is tons and tons of information out there.

TomTom · Accepted Answer · 2014-03-18 14:57:29Z

0

What do you care?

Do not get me wrong - but this is just the generation of a handfull of objects. The real time issue will be the db execution time. So, unless you can profile a problem here it is quite likely youa re totally in premature optimization here. I would accept the code as it is.

answered Mar 18, 2014 at 14:57

TomTom

62.4k11 gold badges95 silver badges152 bronze badges

1 Comment

Alfero Chingono Over a year ago

I thought so too, but then I figured... doesn't hurt to ask.

vandango · Accepted Answer · 2014-03-18 15:02:07Z

0

You could create a new Method with the list (customerAddresses) and the address model (addressModel) as parameter with returns a boolean value to replace "Any".

private bool CheckSync(var customerAddresses, var addressModel)
{
    foreach(var item in customerAddresses)
    {
        if(item.Id != addressModel.Id 
        || !item.Street.Equals(addressModel.Street, StringComparison.OrdinalIgnoreCase)
        || !item.City.Equals(addressModel.City, StringComparison.OrdinalIgnoreCase)
        || !item.Province.Equals(addressModel.Province, StringComparison.OrdinalIgnoreCase)
        || !item.PostalCode.Equals(addressModel.PostalCode, StringComparison.OrdinalIgnoreCase))
        {
            return false;
        }
    }

    return true;
}

And the call this method instead of "Any":

// check if customer already has this address
if (!CheckSync(customerAddresses, addressFilter))
{
    // ...

answered Mar 18, 2014 at 15:02

vandango

5671 gold badge4 silver badges13 bronze badges

3 Comments

Alfero Chingono Over a year ago

I thought Any has more benefits than a loop?

Servy Over a year ago

You have now completely removed any possibility for this query to be performed on the database, and forced it to be done in memory. Also, this doesn't actually answer the question that is asked.

Servy Over a year ago

@vandango It returns true if there is at least one item in the sequence for which the predicate is true.

Collectives™ on Stack Overflow

Optimize LINQ query inside a loop

3 Answers 3

3 Comments

1 Comment

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

1 Comment

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related