simc / dartx

Superpowers for Dart. Collection of useful static extension methods.
https://pub.dev/packages/dartx
Apache License 2.0
1.08k stars 88 forks source link

Add positional `isAscending` to sortedBy #176

Open arthurbcd opened 2 months ago

arthurbcd commented 2 months ago

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.

passsy commented 2 months ago

What's wrong with sortedByDescending()?

arthurbcd commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

I'm fine with landing this

One semantic question:

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

arthurbcd commented 1 month ago

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.