4e15f3d653
* Modify S3169: Add benchmarks * Update rules/S3169/csharp/rule.adoc Co-authored-by: Sebastien Marichal <sebastien.marichal@sonarsource.com> * Update benchmark * LAYC * diff-id --------- Co-authored-by: Sebastien Marichal <sebastien.marichal@sonarsource.com>
140 lines
4.2 KiB
Plaintext
140 lines
4.2 KiB
Plaintext
== Why is this an issue?
|
|
|
|
There's no point in chaining multiple `OrderBy` calls in a LINQ; only the last one will be reflected in the result because each subsequent call completely reorders the list. Thus, calling `OrderBy` multiple times is a performance issue as well, because all of the sorting will be executed, but only the result of the last sort will be kept.
|
|
|
|
== How to fix it
|
|
|
|
Instead, use `ThenBy` for each call after the first.
|
|
|
|
=== Code examples
|
|
|
|
==== Noncompliant code example
|
|
|
|
[source,csharp,diff-id=1,diff-type=noncompliant]
|
|
----
|
|
var x = personList
|
|
.OrderBy(person => person.Age)
|
|
.OrderBy(person => person.Name) // Noncompliant
|
|
.ToList(); // x is sorted by Name, not sub-sorted
|
|
----
|
|
|
|
|
|
==== Compliant solution
|
|
|
|
[source,csharp,diff-id=1,diff-type=compliant]
|
|
----
|
|
var x = personList
|
|
.OrderBy(person => person.Age)
|
|
.ThenBy(person => person.Name)
|
|
.ToList();
|
|
----
|
|
|
|
== Resources
|
|
|
|
=== Benchmarks
|
|
|
|
[options="header"]
|
|
|===
|
|
| Method | Runtime | Mean | StdDev | Allocated
|
|
| OrderByAge | .NET 9.0 | 12.84 ms | 0.804 ms | 1.53 MB
|
|
| OrderByAgeOrderBySize | .NET 9.0 | 24.08 ms | 0.267 ms | 3.05 MB
|
|
| OrderByAgeThenBySize | .NET 9.0 | 18.58 ms | 0.747 ms | 1.91 MB
|
|
| | | | |
|
|
| OrderByAge | .NET Framework 4.8.1 | 22.99 ms | 0.228 ms | 1.53 MB
|
|
| OrderByAgeOrderBySize | .NET Framework 4.8.1 | 44.90 ms | 0.581 ms | 4.3 MB
|
|
| OrderByAgeThenBySize | .NET Framework 4.8.1 | 31.72 ms | 0.402 ms | 1.91 MB
|
|
|===
|
|
|
|
==== Glossary
|
|
|
|
* https://en.wikipedia.org/wiki/Arithmetic_mean[Mean]
|
|
* https://en.wikipedia.org/wiki/Standard_deviation[Standard Deviation]
|
|
|
|
The results were generated by running the following snippet with https://github.com/dotnet/BenchmarkDotNet[BenchmarkDotNet]:
|
|
|
|
[source,csharp]
|
|
----
|
|
public class Person
|
|
{
|
|
public string Name { get; set; }
|
|
public int Age { get; set; }
|
|
public int Size { get; set; }
|
|
}
|
|
|
|
private Random random = new Random(1);
|
|
private Consumer consumer = new Consumer();
|
|
private Person[] array;
|
|
|
|
[Params(100_000)]
|
|
public int N { get; set; }
|
|
|
|
[GlobalSetup]
|
|
public void GlobalSetup()
|
|
{
|
|
array = Enumerable.Range(0, N).Select(x => new Person
|
|
{
|
|
Name = Path.GetRandomFileName(),
|
|
Age = random.Next(0, 100),
|
|
Size = random.Next(0, 200)
|
|
}).ToArray();
|
|
}
|
|
|
|
[Benchmark(Baseline = true)]
|
|
public void OrderByAge() =>
|
|
array.OrderBy(x => x.Age).Consume(consumer);
|
|
|
|
[Benchmark]
|
|
public void OrderByAgeOrderBySize() =>
|
|
array.OrderBy(x => x.Age).OrderBy(x => x.Size).Consume(consumer);
|
|
|
|
[Benchmark]
|
|
public void OrderByAgeThenBySize() =>
|
|
array.OrderBy(x => x.Age).ThenBy(x => x.Size).Consume(consumer);
|
|
----
|
|
|
|
Hardware configuration:
|
|
|
|
[source]
|
|
----
|
|
BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5247/22H2/2022Update)
|
|
Intel Core Ultra 7 165H, 1 CPU, 22 logical and 16 physical cores
|
|
[Host] : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256
|
|
.NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX2
|
|
.NET Framework 4.8.1 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256
|
|
----
|
|
|
|
ifdef::env-github,rspecator-view[]
|
|
|
|
'''
|
|
== Implementation Specification
|
|
(visible only on this page)
|
|
|
|
=== Message
|
|
|
|
Use "ThenBy" instead.
|
|
|
|
|
|
'''
|
|
== Comments And Links
|
|
(visible only on this page)
|
|
|
|
=== on 30 Jun 2015, 13:51:54 Ann Campbell wrote:
|
|
\[~tamas.vajk] I wonder if this is also an efficiency issue. The end result is a list that is sorted only by the last OrderBy argument, but doe all the previous OrderBy's take place, then get thrown away? If so, this would probably be worth adding to the description.
|
|
|
|
=== on 30 Jun 2015, 13:55:28 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] I added a performance related sentence.
|
|
|
|
=== on 30 Jun 2015, 14:52:39 Ann Campbell wrote:
|
|
I shuffled the text some, [~tamas.vajk]
|
|
|
|
=== on 1 Jul 2015, 06:40:10 Tamas Vajk wrote:
|
|
\[~ann.campbell.2] Shouldn't this issue have some performance related label as well?
|
|
|
|
|
|
I simplified the message as the ordering might not happen by some property, but by some complex logic, and in this case we can't display the whole expression and `Comparer` in the message.
|
|
|
|
=== on 1 Jul 2015, 11:26:48 Ann Campbell wrote:
|
|
added [~tamas.vajk]
|
|
|
|
endif::env-github,rspecator-view[]
|