Является ли плохой практикой намеренно полагаться на побочные эффекты Linq?

Нет, это не является строго "плохой практикой" (например, построение SQL-запросов с конкатенацией строк пользовательского ввода или использованием goto).

Иногда такой код более читабелен, чем несколько запросов/foreach или вызов Aggregate без побочных эффектов. Также неплохо хотя бы попытаться написать версии foreach и версии без побочных эффектов, чтобы увидеть, какая из них более читабельна/легче доказать правильность.

Обратите внимание, что:

• часто очень трудно понять, что/когда произойдет с таким кодом. т.е. вы пробуете хаки вокруг факта ленивого выполнения запросов LINQ с вызовом .ToList(), иначе это значение не будет вычислено.
• чистые функции могут выполняться параллельно, если есть побочные эффекты, для этого нужно быть очень осторожным
• если вам когда-нибудь понадобится преобразовать LINQ-to-Object в LINQ-to-SQL, вам придется переписать такие запросы
• обычно запросы LINQ отдают предпочтение функциональному стилю программирования без побочных эффектов (и, следовательно, по соглашению читатели не ожидают побочных эффектов в коде).

Почему бы просто не закодировать это так:

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);

Я бы посчитал это плохой практикой. Вы делаете предположение, что лямбда-выражение принудительно выполняется, потому что вы вызвали ToList. Это деталь реализации текущей версии ToList. Что, если изменить ToList в .NET 7.x, чтобы он возвращал объект, который полулениво преобразует IQueryable? Что, если он изменился для параллельного запуска лямбды? Внезапно у вас возникли проблемы с параллелизмом на вашем staleCount. Насколько я знаю, обе эти возможности могут сломать ваш код из-за неправильных предположений, которые делает ваш код.

Теперь, что касается многократного вызова MultiMerger.GetMerger с одним идентификатором, это действительно должно быть переработано, чтобы быть соединением, поскольку логика выполнения соединения (w|c) будет намного эффективнее, чем то, что вы там закодировали, и будет масштабировать намного лучше, особенно если реализация MultiMerger фактически извлекает данные из базы данных (или может быть изменена для этого).

Что касается вызова ToList() перед передачей его в источник данных, если источник данных не использует все поля в вашем новом объекте, вы будете (намного) быстрее и займете меньше памяти, чтобы пропустить ToList и позволить источнику данных только извлекать нужные ему поля. То, что вы сделали, — это строгое связывание данных с точными требованиями представления, чего следует избегать, где это возможно. Например, что делать, если вам вдруг нужно отобразить поле, которое существует в FileDatabaseMerger, но не находится в вашем текущем анонимном объекте? Теперь вам нужно внести изменения как в контроллер, так и в представление, чтобы добавить его, где, если бы вы просто передали IQueryable, вам нужно было бы изменить только представление. Опять же, быстрее, меньше памяти, более гибкое и более удобное в сопровождении.

Надеюсь, это поможет .. И этот вопрос действительно должен быть опубликован для проверки кода, а не для stackoverflow.

Обновление после дальнейшего рассмотрения, следующий код был бы намного лучше:

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);