-
Notifications
You must be signed in to change notification settings - Fork 19
add ascending
to IterableExtension.sortedBy
#731
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial inclination is to not add this - I don't think we have a similar argument in other sorting methods, we instead expect callers to arrange an appropriate compare callback.
Here I think it's better to ask users to call sortedByCompare
for this use case.
cc @lrhn WDYT?
K Function(T element) keyOf, { | ||
bool ascending = true, | ||
}) { | ||
int compare(K a, K b) => ascending ? a.compareTo(b) : b.compareTo(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the risk of premature optimization, I think we should keep this as a static tear-off instead of a new lambda on every use. If we decide to go forward with this change I think something like
final compare = ascending ? compareComparable : compareComparableDescending;
int compareComparableDescending<T extends Comparable<T>>(T a, T b) => b.compareTo(a);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's typed, it's a new closure on every use anyway, because generic instantiation also causes a new closure.
That said: I don't want to do the branch on every comparison, so something like what @natebosch says is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's an OK feature.
I would reverse the parameter name (to make he default value false
) and give every sorting function the same treatment (if possible).
/// when [ascending] is `false`. | ||
List<T> sortedBy<K extends Comparable<K>>( | ||
K Function(T element) keyOf, { | ||
bool ascending = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer to name it descending
, so the default is false
.
Just because it's usually best to have "nothing" mean false
.
Then it feels like you are opting in to the non-default behavior. Using ascending: false
feels ... conflicted. You are opting out of the default behavior, but that doesn't directly say what it opts in to.
K Function(T element) keyOf, { | ||
bool ascending = true, | ||
}) { | ||
int compare(K a, K b) => ascending ? a.compareTo(b) : b.compareTo(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's typed, it's a new closure on every use anyway, because generic instantiation also causes a new closure.
That said: I don't want to do the branch on every comparison, so something like what @natebosch says is better.
@@ -71,10 +70,15 @@ extension IterableExtension<T> on Iterable<T> { | |||
/// Creates a sorted list of the elements of the iterable. | |||
/// | |||
/// The elements are ordered by the natural ordering of the | |||
/// property [keyOf] of the element. | |||
List<T> sortedBy<K extends Comparable<K>>(K Function(T element) keyOf) { | |||
/// [keyOf] property when [ascending] is `true` or reverse ordering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sorted
function should get the same behavior. Really, every sort
function should behave the same way if at all possible.
(It's probably impossible because there is some sort([int Function(T, T) compare])
somewhere which prevents a named parameter.)
sortedBy
is one of the best extensions of this package.It allows us to easily sort, but we can't set if the order comparaison is ascending/descending, it always defaults to "natural ordering".
This PR simplifies this process so we can simply decide it based on the added
ascending
parameter in thesortedBy
extension.This is useful, because without it, we would have to do additional operations bringing either more boilerplate or additional computations. By adding this named boolean we can make it straightforward.
For many developers, especially those who might not be as familiar with comparators and more advanced functional programming concepts, having a straightforward boolean parameter makes the API more accessible and user-friendly.
The boolean
ascending
is a natural state. It’s a common need to toggle sort order, and doing so with a single extension method and a simple parameter makes the code more readable and maintainable. Adopting this familiar pattern insortedBy
makes it feel more intuitive and consistent, making the intent of the code clearer.Flutter itself uses this pattern for sorting, as we can see in
flutter/lib/src/material/data_table.dart
:I find this extension incredibly useful and have always relied on a version of it. I dedicated time to updating it for the community because I believe it addresses a fundamental and widely shared need across projects.
Thank you for taking the time to review this PR—I hope it proves valuable!
Added tests ✅
Retrocompatible ✅
Not a breaking change ✅
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.