CODEWITHSUNDEEP
c#performancelinq
Belliez
Belliez

Reputation: 5386

inefficient Linq statement

I have the following code to extract customer address as Latitude/Longitude points from an sql database and display points on a winforms Map. However, the database is quite large and this takes a very long time to simply complete the foreach.

Is there a better way to do this?

public static List<MyAppMaps.MapPoints> GetAddresses()
{
    List<MyAppMaps.MapPoints> addresses = new List<MyAppMaps.MapPoints>();

    try
    {
        using (DataClassesDataContext dataClassesDataContext = new DataClassesDataContext(cDbConnection.GetConnectionString()))
        {
            var query = (from customer in dataClassesDataContext.Customers
                where (customer.Address.Latitude != ""
                       || customer.Address.Longitude != "")
                select customer);

            if (query.ToList().Count > 0)
            {
                foreach (Customer item in query)
                {
                    MyAppMaps.MapPoints address = new MyAppMaps.MapPoints();

                    address.Lat = item.Address.Latitude;
                    address.Long = item.Address.Longitude;
                    addresses.Add(address);
                }
            }
        }
    }
    catch (Exception)
    {

    }

    return addresses;
}

Upvotes: 1

Views: 226

Answers (2)

Martin Zikmund
Martin Zikmund

Reputation: 39102

The problem is that you access Address property, which probably points to a different table, which is then lazily loaded during the foreach (meaning each iteration causes another query).

That is something you need to make sure to avoid when working with Entity Framework. If you directly want to include something in the response, use Include method to avoid additional queries under-the-hood. However, usually it is best to use Select projection to return really what you actually need, which will keep the data transfer at the lowest possible rate.

You should use Select to return just the address you need:

var query = (from customer in dataClassesDataContext.Customers
                where (customer.Address.Latitude != ""
                       || customer.Address.Longitude != "")
                select customer.Address);

You also call ToList() to check if there are any items. That executes the query unnecessarily. You can either call ToList() right away and then use the created list, or just use Any to check if there are actually any items with a light-weight query.

Overall you could improve the code like follows:

using (DataClassesDataContext dataClassesDataContext = new DataClassesDataContext(cDbConnection.GetConnectionString()))
{
    var query = from customer in dataClassesDataContext.Customers
        where (customer.Address.Latitude != ""
               || customer.Address.Longitude != "")
        select new MyAppMaps.MapPoints() { Longitude = customer.Address.Longitude, Latitude = customer.Address.Latitude };
    addresses.AddRange( query );
}

Update

As suggested by @Evk, you can improve even further by just returning the two required fields instead of the whole Address instance. I have updated the code to reflect this.

Update 2

Simplified even more based on @Sivaprasath's suggestion - adding the results to the address list via a LINQ query instead.

Upvotes: 1

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 727077

You can do a projection right in the LINQ to avoid pulling the data that you don't need:

var addresses = dataClassesDataContext.Customers.
    .Select(c => c.Address)
    .Where(a => a.Latitude != "" || a.Longitude != "")
    .Select(c => new {
        a.Latitude
    ,   a.Longitude
    }).AsEnumerable() // From this point on LINQ is in memory
    .Select(p => new MyAppMaps.MapPoints {
        Lat = p.Latitude, Long = p.Longitude
    }).ToList();

This simplified version should work as well:

var addresses = dataClassesDataContext.Customers.
    .Select(c => c.Address)
    .Where(a => a.Latitude != "" || a.Longitude != "")
    .Select(a => new MyAppMaps.MapPoints {
        Lat = a.Latitude, Long = a.Longitude
    }).ToList();

Upvotes: 2

Related Questions