Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ImmutableArray extension methods for ordering #10608

Merged
merged 8 commits into from
Jul 16, 2024

Conversation

DustinCampbell
Copy link
Member

This change introduces four sets of extension methods with overloads:

  • OrderAsArray(...)
  • OrderDescendingAsArray(...)
  • OrderByAsArray(...)
  • OrderByDescendingAsArray(...)

Each of these operates on an ImmutableArray<T> and returns an ImmutableArray<T>.

This change introduces four sets of extension methods with overloads:

- OrderAsArray(...)
- OrderDescendingAsArray(...)
- OrderByAsArray(...)
- OrderByDescendingAsArray(...)

Each of these operates on an `ImmutableArray<T>` and returns an `ImmutableArray<T>`.
@DustinCampbell DustinCampbell requested review from a team as code owners July 10, 2024 22:33
Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than general YAGNI, I have nothing against this PR... but didn't I just see a comment on another PR that mentioned there was a Sort method on ImmutableArray already?

It's odd to me that these exist if there is a Sort method, and/or that these don't use that Sort method.

What am I missing?

@DustinCampbell
Copy link
Member Author

DustinCampbell commented Jul 11, 2024

Other than general YAGNI, I have nothing against this PR... but didn't I just see a comment on another PR that mentioned there was a Sort method on ImmutableArray already?

The primary reason is that OrderBy is really useful, especially when you want to sort by some key. ImmutableArray's Sort would require an IComparer<T> or Comparison<T> to sort by a key, so it can often be inconvenient. Also, with ImmutableArray flowing around a lot more through type-inferred variables. It's just too easy to use the LINQ OrderBy without noticing that you've boxed the ImmutableArray and transforming to Sort can change the calling code significantly. So, I understand the feeling that this is YAGNI, but I think #10598 is a good example of why it's useful.

It's odd to me that these exist if there is a Sort method, and/or that these don't use that Sort method.

OrderBy could use the Sort method, but that would mean creating a new IComparer<T> each call to wrap the keyComparer. Then, the keyComparer would need to be called many, many times on the x and y values in each Compare call. Since the runtime provides an optimized method for setting an array of keys and items, I opted to use that and only call keyComparer once for each item. For Order, I think using Sort is a fair point, but it would be problematic for OrderDescending.

1. Don't sort if the array has zero or 1 element.
2. Don't sort if the array or the keys are already ordered.
3. Avoid creating an `IComparer<T>` until its time to actually sort

Add tests for Comparison<T> overloads
@DustinCampbell
Copy link
Member Author

@davidwengier / @ryzngard / @333fred - Feel free to take another look. The implementation has changed pretty significantly.

@DustinCampbell DustinCampbell merged commit 87b4727 into dotnet:main Jul 16, 2024
12 checks passed
@DustinCampbell DustinCampbell deleted the immutablearray-orderby branch July 16, 2024 01:14
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 16, 2024
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 Preview 1 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants