Is it bad practice to purposely rely on Linq Side Effects?

Question

A programming pattern like this comes up every so often:

int staleCount = 0;

fileUpdatesGridView.DataSource = MultiMerger.TargetIds
    .Select(id =>
    {
        FileDatabaseMerger merger = MultiMerger.GetMerger(id);

        if (merger.TargetIsStale)
            staleCount++;

        return new
        {
            Id = id,
            IsStale = merger.TargetIsStale,
            // ...
        };
    })
    .ToList();

fileUpdatesGridView.DataBind();
fileUpdatesMergeButton.Enabled = staleCount > 0;

I'm not sure there is a more succinct way to code this?

Even if so, is it bad practice to do this?

Answer 1

No, it is not strictly "bad practice" (like constructing SQL queries with string concatenation of user input or using goto).

Sometimes such code is more readable than several queries/foreach or no-side-effect Aggregate call. Also it is good idea to at least try to write foreach and no-side-effect versions to see which one is more readable/easier to prove correctness.

Please note that:

• it is frequently very hard to reason what/when will happen with such code. I.e. you sample hacks around the fact of LINQ queries executed lazily with .ToList() call, otherwise that value will not be computed.
• pure functions can be run in parallel, once with side effects need a lot of care to do so
• if you ever need to convert LINQ-to-Object to LINQ-to-SQL you have to rewrite such queries
• generally LINQ queries favor functional programming style without side-effects (and hence by convention readers would not expect side-effects in the code).

Answer 2

Why not just code it like this:

var result=MultiMerger.TargetIds
    .Select(id =>
    {
        FileDatabaseMerger merger = MultiMerger.GetMerger(id);

        return new
        {
            Id = id,
            IsStale = merger.TargetIsStale,
            // ...
        };
    })
    .ToList();
fileUpdatesGridView.DataSource = result;
fileUpdatesGridView.DataBind();
fileUpdatesMergeButton.Enabled = result.Any(r=>r.IsStale);

I would consider this a bad practice. You are making the assumption that the lambda expression is being forced to execute because you called ToList. That's an implementation detail of the current version of ToList. What if ToList in .NET 7.x is changed to return an object that semi-lazily converts the IQueryable? What if it's changed to run the lambda in parallel? All of a sudden you have concurrency issues on your staleCount. As far as I know, both of those are possibilities which would break your code because of bad assumptions your code is making.

Now as far as repeatedly calling MultiMerger.GetMerger with a single id, that really should be reworked to be a join as the logic for doing a join (w|c)ould be much more efficient than what you have coded there and would scale a lot better, especially if the implementation of MultiMerger is actually pulling data from a database (or might be changed to do so).

As far as calling ToList() before passing it to the Datasource, if the Datasource doesn't use all the fields in your new object, you would be (much) faster and take less memory to skip the ToList and let the datasource only pull the fields it needs. What you've done is highly couple the data to the exact requirements of the view, which should be avoided where possible. An example would be what if you all of a sudden need to display a field that exists in FileDatabaseMerger, but isn't in your current anonymous object? Now you have to make changes to both the controller and view to add it, where if you just passed in an IQueryable, you would only have to change the view. Again, faster, less memory, more flexible, and more maintainable.

Hope this helps.. And this question really should be posted of code review, not stackoverflow.

Update on further review, the following code would be much better:

var result=MultiMerger.GetMergersByIds(MultiMerger.TargetIds);
fileUpdatesGridView.DataSource = result;
fileUpdatesGridView.DataBind();
fileUpdatesMergeButton.Enabled = result.Any(r=>r.TargetIsStale);

or

var result=MultiMerger.GetMergers().Where(m=>MultiMerger.TargetIds.Contains(m.Id));
fileUpdatesGridView.DataSource = result;
fileUpdatesGridView.DataBind();
fileUpdatesMergeButton.Enabled = result.Any(r=>r.TargetIsStale);