Do Not Write Abstract Loops

Instead make API consumers implement their own looping logic.

Following my post on Maintenance Programming, I think I may write a series of posts about coding patterns that I have come across that have caused me much pain.

A very typical pattern is something like this:

var strategy = GetStrategy();
foreach (var item in items)
{
    strategy.Process(item);
}

This might be innocuous, but when I see this I can want to cry. This is an abstract loop. That is, you have a loop that is applying some abstract function to each item in the loop individually. Consumers of the API will define different implementations of the strategy, and apply some side affect or collect some data from the item passed in by the loop.

This pattern must be logical, because I have seen it in many places, particularly in the context of validation and in queue processing. The logic is that we are avoiding code duplication by having the loop implemented in only one place, and keeping strategy.Process simple by having it only take in a trivial parameter.

Why is this bad? It This pattern curses the implementation strategy.Process to be either difficult to read, or inefficient. What the writer of this loop isn't anticipating is that the implementation of strategy.Process actually is looking at all of the other items, or is loading something from the database that could be loaded in a batch. There is almost a 100% certainty that there will be some hidden O(n) cost inside of strategy.Process. Some poor bastard will be sent in to fix this.

If only the original implemented had done it like this:

GetStrategy().Process(items);

Then the maintainer could trivially cache or batch shared dependencies of the items improving performance!

Here is another example of this pattern:

foreach (var row in rows)
{
    row.Validate();
}

Once again, you have a problem. If the implementation of row.Validate() looks at other rows your have a hidden O(n^2) cost that will ruin some poor maintenance programmers day as soon has your product hits scale.

Once again, the solution is to avoid abstracting your loops:

RowValidator.Validate(rows);

Or if you need polymorphism based on the type of row, then at the very least do something like this:

foreach (var group in rows.GroupBy(r => r.GetType())
{
    GetValidator(group.Key).Validate(group);
}