“Developers don’t make simple mistakes” on the example of sorts in Unity, ASP.NET Core and not only
There is an opinion that experienced developers do not make simple mistakes. Comparison errors? Null reference dereferencing? No, it’s definitely not about us… 😉 By the way, what about sorting errors? As you already understood from the title, this also has nuances.
OrderBy(…).OrderBy(…)
There is such an error pattern in C# – when sorting a collection, the method is called twice in a row OrderBy. The intention of the developer is simple – to perform a secondary sort, keeping the results of the primary.
The easiest way to explain the problem is with an example. Let’s say we have some type (Wrapper) with two integer properties (Primary And secondary). There is an array of instances of this type that needs to be sorted in ascending order, first by the primary key, then by the secondary.
The code:
class Wrapper
{
public int Primary { get; init; }
public int Secondary { get; init; }
}
var arr = new Wrapper[]
{
new() { Primary = 1, Secondary = 2 },
new() { Primary = 0, Secondary = 1 },
new() { Primary = 2, Secondary = 1 },
new() { Primary = 2, Secondary = 0 },
new() { Primary = 0, Secondary = 2 },
new() { Primary = 0, Secondary = 3 },
};
var sorted = arr.OrderBy(p => p.Primary)
.OrderBy(p => p.Secondary);
foreach (var wrapper in sorted)
{
Console.WriteLine($"Primary: {wrapper.Primary}
Secondary: {wrapper.Secondary}");
}Unfortunately, the output of this code will be wrong:
Primary: 2 Secondary: 0
Primary: 0 Secondary: 1
Primary: 2 Secondary: 1
Primary: 0 Secondary: 2
Primary: 1 Secondary: 2
Primary: 0 Secondary: 3The sequence was sorted by the secondary key, but not by the primary. If you’ve used “multi-level” sorting in C# at least once, you’ve already guessed what the catch is here.
Yes, method call again OrderBy resets the result of the previous sort. The correct sequence would be OrderBy(…).ThenBy(…):
var sorted = arr.OrderBy(p => p.Primary)
.ThenBy(p => p.Secondary);Then the result of the code will be expected:
Primary: 0 Secondary: 1
Primary: 0 Secondary: 2
Primary: 0 Secondary: 3
Primary: 1 Secondary: 2
Primary: 2 Secondary: 0
Primary: 2 Secondary: 1In the documentation for the method ThenBy on the docs.microsoft.com there is a note on this: Because IOrderedEnumerable
It so happened that recently I went through C # projects on GitHub and drove their code through PVS Studio. The analyzer just has a diagnostic on the topic of misuse OrderBy —V3078.
And what, does anyone make such mistakes? Well, what can I say… 🙂
Examples from open source projects
Unity
In Unity, 2 similar problems were found at once.
First place
private List<T> GetChildrenRecursively(bool sorted = false,
List<T> result = null)
{
if (result == null)
result = new List<T>();
if (m_Children.Any())
{
var children
= sorted ?
(IEnumerable<MenuItemsTree<T>>)m_Children.OrderBy(c => c.key)
.OrderBy(c => c.m_Priority)
: m_Children;
....
}
....
}It’s simple – we wanted to sort the collection m_Children first by key (c.key), then by priority (c.priority). However, the resulting sequence will only be sorted by priority.
Second place
static class SelectorManager
{
public static List<SearchSelector> selectors { get; private set; }
....
internal static void RefreshSelectors()
{
....
selectors
= ReflectionUtils.LoadAllMethodsWithAttribute(
generator,
supportedSignatures,
ReflectionUtils.AttributeLoaderBehavior.DoNotThrowOnValidation)
.Where(s => s.valid)
.OrderBy(s => s.priority)
.OrderBy(s => string.IsNullOrEmpty(s.provider))
.ToList();
}
}This time, providers were the secondary sort key (s.provider). But, as we know, this is the only criterion by which the resulting sequence will be sorted.
I reported both issues via the Unity Bug Reporter. As a result, the QA team opened 2 issues:
There were no comments on them yet – we are waiting.
ASP.NET Core
There were 3 places in ASP.NET Core where calls were duplicated OrderBy. All of them were found in the KnownHeaders.cs file.
First place
RequestHeaders = commonHeaders.Concat(new[]
{
HeaderNames.Authority,
HeaderNames.Method,
....
}
.Concat(corsRequestHeaders)
.OrderBy(header => header)
.OrderBy(header => !requestPrimaryHeaders.Contains(header))
....Second place
ResponseHeaders = commonHeaders.Concat(new[]
{
HeaderNames.AcceptRanges,
HeaderNames.Age,
....
})
.Concat(corsResponseHeaders)
.OrderBy(header => header)
.OrderBy(header => !responsePrimaryHeaders.Contains(header))
....Third place
ResponseTrailers = new[]
{
HeaderNames.ETag,
HeaderNames.GrpcMessage,
HeaderNames.GrpcStatus
}
.OrderBy(header => header)
.OrderBy(header => !responsePrimaryHeaders.Contains(header))
....The pattern is the same everywhere, only the variables used change. All three cases have been described in issue on the project page.
The developers replied that in this case this is not a bug, but the code was fixed. Link to commit with fix.
In any case, it is better to avoid such code – it immediately raises questions.
CosmosOS (IL2CPU)
private Dictionary<MethodBase, int?> mBootEntries;
private void LoadBootEntries()
{
....
mBootEntries = mBootEntries.OrderBy(e => e.Value)
.OrderByDescending(e => e.Value.HasValue)
.ToDictionary(e => e.Key, e => e.Value);
....
}Here we are dealing with a strange sorting by fields of the type int?. I also created issue on this occasion. In this case, re-sorting turned out to be redundant, so the method call OrderByDescending just removed. The commit can be found here.
grandnode
public IEnumerable<IMigration> GetCurrentMigrations()
{
var currentDbVersion = new DbVersion(int.Parse(GrandVersion.MajorVersion),
int.Parse(GrandVersion.MinorVersion));
return GetAllMigrations()
.Where(x => currentDbVersion.CompareTo(x.Version) >= 0)
.OrderBy(mg => mg.Version.ToString())
.OrderBy(mg => mg.Priority)
.ToList();
}We wanted to sort first by version, then by priority. As a result, sorting is performed only by priority.
As in previous cases, opened issue. As a result of the correction, the second call OrderBy changed to ThenBy:
.OrderBy(mg => mg.Version.ToString())
.ThenBy(mg => mg.Priority)The commit with the fix can be found here.
Human factor?
The appearance of complex and complex errors can be explained by a lack of experience. Seems like a challenge OrderBy(…).OrderBy(…) – not one of those. Where do these problems come from then?
Here I have 2 theories:
- the first – still plays a lack of experience. If the code is written by a person who has never encountered this and does not test the results of the work. 🙂
- the second is the human factor. We already know that people tend to make mistakes in comparison functionsexists last line effect, often mistakes are made simply because of copy-paste. Possibly a double call OrderBy – another manifestation of the human factor.
If such problems can escape from the human eye, then from a static analyzer it is already unlikely. It’s great when it is used in an IDE or built into a pipeline, and even better if it is used both there and there.
In general, automate the search for errors. 🙂
In good tradition, I invite you to subscribe to me on Twitterso as not to miss thematic publications.
If you want to share this article with an English-speaking audience, please use the translation link: Sergey Vasiliev. “Developers don’t make silly errors” by the example of sorting in Unity, ASP.NET Core, and more.
