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 positional isAscending to sortedBy #176

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

arthurbcd
Copy link

sortedBy is one of the best extensions of dartx.

SortedList class allows us to sort, and uses the internal order int to decide whether it is ascending or not.

This PR simplifies this process so we can simply decide it based on isAscending positional boolean in the sortedBy extension.

This is useful, because without it, we would have to do additional operations bringing either more boilerplate or additional computations. By adding this positional boolean we can make it straightforward.

  • Added tests ✅
  • Retrocompatible ✅
  • Not a breaking change ✅

@passsy
Copy link
Collaborator

passsy commented Jun 19, 2024

What's wrong with sortedByDescending()?

@arthurbcd
Copy link
Author

Imagine you have manage a state like table sorts.

Today we have to do something like this:

    final classes = isAscending
        ? context.watch<ClassStore>().classes.sortedBy(
              (clazz) => switch (sortType) {
                ClassSort.name => clazz.name,
                ClassSort.code => clazz.code,
                ClassSort.createdAt => clazz.createdAt.toString(),
              },
            )
        : context.watch<ClassStore>().classes.sortedByDescending(
              (clazz) => switch (sortType) {
                ClassSort.name => clazz.name,
                ClassSort.code => clazz.code,
                ClassSort.createdAt => clazz.createdAt.toString(),
              },
            );

Or look for other solutions, like reverse the list, which requires even more computations.

isAscending is a really common state variable, and have them in sortedBy() as a positional or named parameter simplifies this by a lot.

In DataTable widget, for example, we have the DataColumnSortCallback that returns the isAscending boolean, which makes it even more straightforward and intuitive.

Ex:

    final classes = context.watch<ClassStore>().classes.sortedBy(
          (clazz) => switch (sortType) {
            ClassSort.name => clazz.name,
            ClassSort.code => clazz.code,
            ClassSort.createdAt => clazz.createdAt.toString(),
          },
          isAscending, // <- with `isAscending`
        );

This would make the current sortedBy even more flexible and seamless with Flutter. And there are no drawbacks for the current users.

@passsy
Copy link
Collaborator

passsy commented Jun 20, 2024

Especially, when your sort order can be changed, you should use sortedWith and not sortedBy/sortedByDescending as those will produce more code which can be avoided by reversing the Comparator.

List<Clazz> doSort(List<Clazz> classes, ClassSort sortType, bool isAscending) {
  final Comparator<Clazz> comparator = compareBy((clazz) => switch (sortType) {
    ClassSort.name => clazz.name,
    ClassSort.code => clazz.code,
    ClassSort.createdAt => clazz.createdAt.toString(),
  });
  return classes.sortedWith(isAscending ? comparator : comparator.reverse());
}

/// Creates a comparator using the function to transform value to a [Comparable] instance for comparison.
Comparator<T> compareBy<T>(Comparable Function(T) selector) {
  int compareTo(T a, T b) => selector(a).compareTo(selector(b));
  return compareTo;
}

I took compareBy from kt_dart to stick closely to your code. But there are other ways to create a Comparator.

You can find this very example in my "Getting started with Functional Programming" talk from 2022.

@arthurbcd
Copy link
Author

Thank you for the detailed explanation and the alternative approach using sortedWith/Comparator.reverse(). I appreciate the efficiency and elegance of this method, and I understand it aligns well with functional programming principles as mentioned in the video!

I still believe there’s a strong case for adding the isAscending parameter to sortedBy for one main reason: Simplicity.

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 isAscending 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 in sortedBy makes it feel more intuitive and consistent, making the intent of the code clearer.

I appreciate your consideration of this proposal. I really believe it can add value to dartx by enhancing its usability and aligning it with common patterns, making it easier for developers to adopt and use effectively. I even talked with my team, and they encouraged me to do this PR.

Thanks again for your time and for the great work on this package. Loved watching your video, you are inspiring :).

@arthurbcd
Copy link
Author

Refined parameter naming and documentation for consistency with Flutter conventions.

  SortedList<E> sortedBy(
    Comparable Function(E element) selector, {
    bool ascending = true,
  }) {
   expect([4, 2, 1, 3].sortedBy((it) => it, ascending: true), [1, 2, 3, 4]);
   expect([4, 2, 1, 3].sortedBy((it) => it, ascending: false), [4, 3, 2, 1]);

@passsy
Copy link
Collaborator

passsy commented Jul 24, 2024

I'm fine with landing this

One semantic question:

  • Do we want a bool ascending or do we want an enum SortOrder order = {SortOrder.asc, SortOrder.desc}.

When sortedBy received the sort order parameter then thenBy should receive the same, too.

@arthurbcd
Copy link
Author

I'd say ascending boolean as Flutter uses this pattern:

/// Signature for [DataColumn.onSort] callback.
typedef DataColumnSortCallback = void Function(int columnIndex, bool ascending);

Reference: data_table.dart.

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.

2 participants