The Rocky Road to Agility

A few months ago, I wrote about the beginning of our journey on the path to agility. Since then, we’ve made some major strides and also had some serious set backs.

Our process was getting better, but we continued to struggle with the legacy code base. I was writing tests and refactoring the code base, but was still the only one doing so. The team was seeing the benefits I was getting out of it, but simply didn’t know how to do it themselves.

Be Careful with ToList()

I recently came across some code during a review that seemed perfectly fine at first glance, but upon further inspection, had potential to perform terribly. Take a look. What’s wrong with this code?

using (var context = new DbContext())
{
context.SomeTable.Where(t => t.Id == someId)
.GroupBy(t => t.Category)
.Select(tg => new { tg.Category, Profit = tg.Sum(p => p.Profit) })
.ToList()
.ForEach(SaveToDb);
}

Do you see it? It’s easy to miss.

Calling ToList() on the Enumerable materializes the query, iterating over the Enumerable in order to generate the List. Then, just a moment later, we iterate over the List. We’ve potentially doubled the run time of this method. In most cases, likely not a big deal, but what if the List contains thousands of items? We’re doing more work than necessary here.

Fixing this is a simple matter of storing the IEnumerable in a variable and using the traditional foreach syntax instead of the ForEach extension method.

using (var context = new DbContext())
{
var query = context.SomeTable.Where(t => t.Id == someId)
.GroupBy(t => t.Category)
.Select(tg => new { tg.Category, Profit = tg.Sum(p => p.Profit) });

foreach(var item in query)
{
SaveToDb(item);
}
}

It just goes to show that you really have to be paying attention when reviewing (or writing!) code. Some issues can be terribly difficult to spot at a glance.