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

Core fn: sort-by #63

Closed
lilactown opened this issue Aug 18, 2022 · 2 comments
Closed

Core fn: sort-by #63

lilactown opened this issue Aug 18, 2022 · 2 comments

Comments

@lilactown
Copy link
Collaborator

Implement sort-by

See #22 for implementation details of sequence functions

@corasaurus-hex
Copy link
Collaborator

This one is tricky. Whose sorting do we want to match, javascript's or clojurescript's?

Comparing JS vs CLJS Sort

  1. In js there is no sortBy but there is sort in which the "default sort order is ascending, built upon converting the elements into strings, then comparing their sequences of UTF-16 code units values." This leads to some surprising behaviors:
const array1 = [1, 30, 4, 21, 100000];
array1.sort();
console.log(array1);
// expected output: Array [1, 100000, 21, 30, 4]
  1. In cljs the behavior is a lot less surprising but it is built on compare which "Uses IComparable if available and google.array.defaultCompare for objects of the same type. nil is treated as a special case and is always less than any other object." Here's the source of compare:
(defn ^number compare
  [x y]
  (cond
   (identical? x y) 0

   (nil? x) -1

   (nil? y) 1

   (number? x) (if (number? y)
                 (garray/defaultCompare x y)
                 (throw (js/Error. (str "Cannot compare " x " to " y))))

   (satisfies? IComparable x)
   (-compare x y)

   :else
   (if (and (or (string? x) (array? x) (true? x) (false? x))
            (identical? (type x) (type y)))
     (garray/defaultCompare x y)
     (throw (js/Error. (str "Cannot compare " x " to " y))))))

And google.array.defaultCompare is simple enough and what I'd write if I was just sorting numbers:

function defaultCompare(a, b) {
  return a > b ? 1 : a < b ? -1 : 0;
}

Implementation

The only real difference between the two implementations is the compare function. The actual sort-by function could look like this:

export function sort_by(keyFn, compFnOrColl, collIfCompFn = undefined) {
  const compFn = collIfCompFn ? compFnOrColl : defaultCompare;
  const coll = collIfCompFn ? collIfCompFn : compFnOrColl;

  // sort mutates the array in place and so we need to spread it into a new
  // array first
  return [...iterable(coll)].sort((a, b) => compFn(keyFn(a), keyFn(b)));
}

Whichever direction we go we should use that same compare function for the sort function as well.

  1. If we want to match javascript's compare behavior it would look like this:
function defaultCompare(a, b) {
  // default js sort uses the same string conversion that
  // template interpolation does, from what I can tell
  const aString = `${a}`;
  const bString = `${b}`;

  if (aString > bString) return 1;
  else if (aString < bString) return -1;
  else return 0;
}
  1. Matching cljs' compare implementation shouldn't be too hard, either. The only missing function, from what I can tell, is number?. We could also add IComparable at the same time as sort-by or we could just punt on that and scrap that part of the cljs compare function.

Gotchas

In either case sparse arrays are an interesting problem. How do we want to deal with them?

Conclusion

I think it would pay off to add IComparable and match cljs' behavior here, since the default behavior of js sort is so weird and not tremendously useful unless you're sorting strings.

@borkdude
Copy link
Member

Added

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

No branches or pull requests

3 participants