weavejester / medley

A lightweight library of useful Clojure functions
Eclipse Public License 1.0
865 stars 66 forks source link

Add least-by & greatest-by fns #69

Closed tomdl89 closed 1 year ago

tomdl89 commented 1 year ago

I've written these a few times over the years, and seen requests for them in clojurians slack too. They nicely complement least and greatest in the same way sort-by complements sort. Edit: I had to rewrite it with a loop (like max-key and min-key) to avoid calling k too often.

weavejester commented 1 year ago

Thanks for the PR. Can you ensure that the docstrings wrap at 80 characters, and I think the line about min-key could be expressed more clearly. Perhaps something like: "If keyfn returns numbers, prefer: clojure.core/min-key".

Also, instead of k, I think the argument would be clearer if it were keyfn. (I know k is used in min-key and max-key, but I think we can improve readability here.)

Finally, it would be nice to have some use-cases to justify these functions.

tomdl89 commented 1 year ago

Thanks for the feedback. I've updated the docstrings for wrapping, and tried to improve the line about min-key but I think it's best to stick to referring to core rather than clojure.core/ because this lib can be used in clojurescript, where it's cljs.core/min-key instead.

Regarding use-cases, basically anywhere where you would do (first (sort-by keyfn coll)) or (last (sort-by keyfn coll)) but you want to avoid O(n log n) time complexity, and prefer the terser syntax. Or where you would do (max-key keyfn coll) or (min-key keyfn coll) but want to compare something other than numbers.

Comparing chars:

(least-by second "abode" "baker" "scald")
;; => "baker"

(min-key second "abode" "baker" "scald")
;; => ClassCastException class java.lang.Character cannot be cast to class java.lang.Number (java.lang.Character and java.lang.Number are in module java.base of loader 'bootstrap')  clojure.lang.Numbers.lt (Numbers.java:226)

Using vectors for multiple-sort:

(greatest-by (juxt :a :b) {:a 5 :b 3} {:a 2 :b 4} {:a 5 :b 7} {:a 5 :b 2})
;; => {:a 5, :b 7}

(max-key (juxt :a :b) {:a 5 :b 3} {:a 2 :b 4} {:a 5 :b 7} {:a 5 :b 2})
;; => ClassCastException class clojure.lang.PersistentVector cannot be cast to class java.lang.Number (clojure.lang.PersistentVector is in unnamed module of loader 'app'; java.lang.Number is in module java.base of loader 'bootstrap')  clojure.lang.Numbers.gt (Numbers.java:234)

Handling nil:

(greatest-by first [6 7] [] [2] [8 9] [4 5 6])
;; => [8 9]

(max-key first [6 7] [] [2] [8 9] [4 5 6])
;; => NullPointerException Cannot invoke "Object.getClass()" because "x" is null  clojure.lang.Numbers.ops (Numbers.java:1018)
tomdl89 commented 1 year ago

@weavejester lemme know if there's anything you'd like changing 🙏

weavejester commented 1 year ago

Thanks for the update on this, and the example use-cases.

I think I'd still prefer to use clojure.core/min-key over "core function min-key", even if it's technically incorrect when referring to ClojureScript.

My reasoning is that the intent is clearer overall; I had to stop and think about what "core function min-key" meant when I read the docstring for the first time, while clojure.core/min-key refers unambiguously to the function in question. While the namespace differs in ClojureScript, I don't think this is confusing, or at least it wouldn't be to me. I recognize this is rather subjective.

It's also the convention used in other docstrings in the same namespace (e.g. deep-merge, update-existing), and consistency is often the tie-breaker for me.

tomdl89 commented 1 year ago

Fair enough - thanks for explaining your thinking. I've pushed the changes as requested 🚀

weavejester commented 1 year ago

Thanks! It looks good. Could you change fns in the commit message to functions? It's a minor thing, but it would help with searching the commit log. Then I'll merge and release 1.6.0.

tomdl89 commented 1 year ago

That's all done. Hopefully now ready to merge @weavejester

tomdl89 commented 1 year ago

Bump @weavejester

weavejester commented 1 year ago

Merged! Sorry for the delay.

tomdl89 commented 1 year ago

Hey @weavejester can you cut a release when you next find the time? I'd like to grab it from clojars for a couple of projects 🙏

weavejester commented 1 year ago

Done.