Longing to know

Recently, I was reviewing some old code to see why it was performing poorly. The code in question was Database specific and was using an Entity Framework 4.4. Of course, many people wanted to immediately blame it all on Entity Framework.

Personally, I’m not so quick to condemn a framework. Frameworks usually do what you tell them to do, but only as well as you instruct them. From my experience, most of the time when a framework behaves badly, it’s not the framework’s fault. It’s usually due to poor utilization and understanding of the framework.

At any rate, I came upon this little bit of code (names have been changed to protect the innocent):

var retList = from x in _context.MyDbSet.Include(x => x.ChildTable)
                where (string.IsNullOrEmpty(prop1) || d.Prop1.Equals(prop1, StringComparison.InvariantCultureIgnoreCase))
                && (string.IsNullOrEmpty(childProp1) || d.ChildTable.Prop1.Equals(childProp1, StringComparison.InvariantCultureIgnoreCase))
                && (string.IsNullOrEmpty(childProp2) || d.ChildTable.Prop2.Equals(childProp2, StringComparison.InvariantCultureIgnoreCase))
                && (string.IsNullOrEmpty(prop2) || d.Prop2.Equals(prop2, StringComparison.InvariantCultureIgnoreCase))
                && (string.IsNullOrEmpty(prop3) || d.Prop3.Equals(prop3, StringComparison.InvariantCultureIgnoreCase))
                orderby x.Id
                select x;

At first glance it looks pretty innocuous, but my first impression was that it had a certain code smell. Since this project was using an extremely old version of Entity Framework, I couldn’t use Entity Framework command interceptors to view the generated SQL. Fortunately, I was able to view the generated SQL with Entity Framework Profiler easily.

The first thing I saw in the generated SQL was something like this in the WHERE clause for each property:

WHERE  (('' /* @p__linq__0 */ IS NULL)
		 OR ((CAST(LEN('' /* @p__linq__0 */) AS int)) = 0)
		 OR ([Extent1].[Prop1] = '' /* @p__linq__1 */))

That’s ugly. I’m also not showing the full extent of the ugliness. One could immediately jump to the conclusion that the ORM Framework is at fault, especially considering the resulting query can take 2+ seconds to execute. I’m fairly certain that newer versions of the EF don’t produce such ugly SQL, but not withstanding, the written code seems to be expecting too much, imho. I’d go so far as to say, even if I were personally translating that to a StoredProc, I wouldn’t follow the same pattern.

Refactoring the code into something a little more explicit seemed to be worth a try:

try
{
    var query = _context.MyDbSet.Include(x => x.ChildTable);

    if (string.IsNullOrWhiteSpace(prop1))
    {
        query = query.Where(x => x.Prop1.Equals(prop1, StringComparison.InvariantCultureIgnoreCase));
    }

    if (!string.IsNullOrWhiteSpace(prop2))
    {
        query = query.Where(x => x.Prop2.Equals(prop2, StringComparison.InvariantCultureIgnoreCase));
    }

    if (!string.IsNullOrWhiteSpace(prop3))
    {
        query = query.Where(x => x.Prop3.Equals(prop3, StringComparison.InvariantCultureIgnoreCase));
    }

    if (!string.IsNullOrWhiteSpace(childProp1))
    {
        query = query.Where(x => x.ChildTable.ChildProp1.Equals(childProp1, StringComparison.InvariantCultureIgnoreCase));
    }

    if (!string.IsNullOrWhiteSpace(childProp2))
    {
        query = query.Where(x => x.ChildTable.ChildProp2.Equals(childProp2, StringComparison.InvariantCultureIgnoreCase));
    }

    if (skip > 0)
    {
        query = query.Skip(skip);
    }

    if (take < Int32.MaxValue)
    {
        query = query.Take(take);
    }

    var retList = query
        .OrderBy(x => x.Id)
        .Select(x => x)
        .ToList();

    return retList;
}
catch (Exception ex)
{
    return null;
}

This simple refactoring of the code reduces query times of 2+ seconds down to less than 10ms. The Framework isn’t different. However, the utilization of EF is different and the execution path becomes more predictable. We’re thinking through the problem slightly differently and I’d call that win-win.