Некоторые считают, что опытные разработчики не делают глупых ошибок. Ошибки сравнения? Разыменовывать нулевые ссылки? Спорим, вы думаете: «Нет, это точно не про меня…» ;) Кстати, а что с ошибками при сортировке? Как следует из названия, есть некоторые нюансы.
ЗаказПо(…).ЗаказПо(…)
Позвольте мне привести пример, чтобы описать проблему. Допустим, у нас есть некоторый тип (Wrapper) с двумя целочисленными свойствами (Primary и Secondary). Существует массив экземпляров этого типа. Нам нужно отсортировать его в порядке возрастания. Сначала — по первичному ключу, затем — по вторичному ключу.
Вот код:
К сожалению, результат этого кода будет неверным:
Последовательность оказалась отсортированной по вторичному ключу. Но сортировка по первичному ключу не сохранилась. Если вы когда-либо использовали многоуровневую сортировку в C#, то можете догадаться, в чем ее подвох.
Второй вызов метода OrderBy вводит новый первичный порядок. Это означает, что вся последовательность будет отсортирована заново.
Но нам нужно зафиксировать результат первичной сортировки. Вторичная сортировка не должна сбрасывать его.
В этом случае правильная последовательность вызовов такова: OrderBy(…).ThenBy(…):
Затем код выдает ожидаемый результат:
У Microsoft естьдокументациядля метода ThenBy. По этому поводу есть примечание:Поскольку IOrderedEnumerable‹TElement› наследуется от IEnumerable‹T›, вы можете вызывать OrderBy или OrderByDescending для результатов вызова OrderBy, OrderByDescending, ThenBy или ThenByDescending. Это вводит новый основной порядок, который игнорирует ранее установленный порядок.
Недавно я просматривал проекты C# на GitHub и выбрал несколько для проверки с помощью PVS-Studio. Анализатор имеет диагностику V3078 о возможном неправильном использовании OrderBy.
Хотите знать, что я нашел? ;)
Примеры из проектов с открытым исходным кодом
Единство
В Unity анализатор обнаружил 2 похожих фрагмента кода.
Первый фрагмент
Возможно, разработчики хотели отсортировать коллекцию m_Children сначала по ключу (c.key), а затем по приоритету (c.priority). Но сортировка по приоритету будет выполняться по всей коллекции. Сортировка по ключу не будет исправлена. Это ошибка? Тут надо спросить у разработчиков.
Второй фрагмент
Результат сортировки в следующем порядке:
- последовательность начинается с элементов с провайдерами. Элементы без провайдеров следуют за ними. Можно сказать, что у нас есть 2 «группы»: с провайдерами и без них;
- в этих группах элементы отсортированы по приоритету.
Пожалуй, здесь нет ошибки. Однако согласитесь, что последовательность вызовов OrderBy().ThenBy() легче читать.
Я сообщил об обеих проблемах через Unity Bug Reporter. После этого Unity QA Team открыла 2 вопроса:
Задачи пока не содержат комментариев. Так что все еще ждем обновлений.
Ядро ASP.NET
PVS-Studio обнаружила 3 места в ASP.NET Core с повторяющимися вызовами OrderBy. Все они были обнаружены в файле KnownHeaders.cs.
Первый выпуск
Вторая проблема
Третья проблема
Паттерн ошибки тот же, отличаются только используемые переменные. Чтобы сообщить об этих проблемах, я создал новую проблему на странице проекта.
Разработчики ответили, что повторяющиеся вызовы OrderBy не являются ошибками. Тем не менее, они исправили код. Найти коммит можно здесь.
В любом случае, я считаю, что так код писать не стоит. Повторяющиеся вызовы OrderBy выглядят очень подозрительно.
КосмосОС (IL2CPU)
Здесь мы имеем дело со странной сортировкой по полям типа int?. Я также создал вопрос для этого. В данном случае вторичная сортировка оказалась избыточной. Вот почему разработчики удалили вызов OrderByDescending. Найти коммит можно здесь.
Великий узел
Возможно, разработчики хотели сделать сортировку сначала по версии, потом — по приоритету. В результате сортировка выполняется только по приоритету.
Как и в предыдущих выпусках, я информировал разработчиков. Они исправили это, заменив второй вызов OrderBy на ThenBy:
Исправление можно найти здесь.
Надежность человека?
Последовательность вызовов OrderBy().OrderBy() не может быть ошибкой. Но такой код вызывает вопросы. Это правильно? Что, если здесь следует использовать OrderBy().ThenBy()?
Как разработчики могут допускать такие ошибки?
Возможно, это человеческая надежность. Мы знаем, что разработчики склонны делать ошибки в функциях сравнения. Также присутствует эффект последней строки. Более того, копипаст часто провоцирует ошибки. Возможно, многократный вызов OrderBy — еще одно проявление человеческой надежности.
В любом случае, будьте осторожны с этим. :)
По доброй традиции приглашаю вас подписаться на меня в Твиттере, чтобы не пропустить интересные публикации.
Напоследок, скажите, пожалуйста: встречались ли вы с подобной картиной?
