Suggestion: LINQ DB analyzers & Fixes
- Dominant language
- C#
- Stars
- 3.5k
- Forks
- 294
- Avg merge
- 2h 30m
- Merged PRs (30d)
- 4
Description
I was recently working with some code that parameterized a query in an arguably over-clever way. The SQL it generated was bloated and complex.
This approximates the "BEFORE" code:
```csharp
private IEnumerable> GetClientInfo(DbContext db)
{
string singleItem = String.IsNullOrWhiteSpace(options.SingleItem) ? String.Empty : options.SingleItem.Trim();
return db.ClientInfo
.Where(
p => p.DateSent == null
&& (!options.ClientList.Any() || options.ClientList.Contains(p.ClientId))
&& (!options.ExcludedClientsList.Any() || !options.ExcludedClientsList.Contains(p.ClientId))
&& (String.IsNullOrEmpty(singleItem) || p.ItemLocator.Equals(singleItem, StringComparison.OrdinalIgnoreCase))
&& (p.SendAfter == null || p.SendAfter < DateTime.Now)
)
.OrderBy(p => p.DateAdded)
.GroupBy(p => p.ClientId).ToList();
}
```
I changed the code by introducing a local for the query and then breaking out the `&&`'s into additional chained `Where` calls, each guarded with an appropriate condition. The AFTER code is a little longer, but the resulting SQL is simpler because of the parts that are no longer necessary when the conditions are right.
The "AFTER" code looks something like this:
```csharp
private IEnumerable> GetClientInfo(DbContext db)
{
string singleItem = String.IsNullOrWhiteSpace(options.SingleItem) ? String.Empty : options.SingleItem.Trim();
IQueryable query =
db.ClientInfo
.Where(p => p.DateSent == null && (p.SendAfter == null || p.SendAfter < DateTime.Now));
if (options.ClientList.Any())
{
query = query.Where(p => options.ClientList.Contains(p.ClientId));
}
if (options.ExcludedClientsList.Any())
{
query = query.Where(p => !options.ExcludedClientsList.Contains(p.ClientId));
}
if (!String.IsNullOrEmpty(singleItem))
{
query = query.Where(p => p.ItemLocator == singleItem); // Db string comparison is already case-insensitive
}
return query
.OrderBy(p => p.DateAdded)
.GroupBy(p => p.ClientId)
.ToList();
}
```
The options.ClientList and options.ExcludedClientsList are properties that are in-memory collections. For the purpose of the query, the "singleItem" local is simple static text. With the lazy nature of IQueryable queries, that `.ToList()` at the end may make this case unique. Also, I haven't discounted the possibility that the list properties could be `null`, in which case my conditionals might need to change accordingly. This could also be an incredibly narrow analyzer niche to fill.
Regardless, even though I can intuit the pattern and manually apply it here, it would be really nice to have automated tool to break it apart like this. This request could be a forerunner of a whole family of LINQ related analyzers and refactorings based on interactions between `IEnumerable` and `IQueryable`. Interesting?
Contributor guide
Assessment
This issue has not been assessed yet.