-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Optimise NonEmptyTraverse implementation #3382
Optimise NonEmptyTraverse implementation #3382
Conversation
tail.headOption.fold(Eval.now(Apply[G].map(f(head))(NonEmptyLazyList(_)))) { h => | ||
Apply[G].map2Eval(f(head), Eval.defer(loop(h, tail.tail)))((b, acc) => b +: acc) | ||
} |
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.
There is a bit of LISP here. This code may be more readable as follows:
tail.headOption.fold(Eval.now(Apply[G].map(f(head))(NonEmptyLazyList(_)))) { h => | |
Apply[G].map2Eval(f(head), Eval.defer(loop(h, tail.tail)))((b, acc) => b +: acc) | |
} | |
val fh = f(head) | |
tail match { | |
case _ if tail.isEmpty => | |
Eval.now(Apply[G].map(fh)(NonEmptyLazyList(_))) | |
case h #:: ttail => | |
val ftail = Eval.defer(loop(h, ttail)) | |
Apply[G].map2Eval(fh, ftail)(_ +: _) | |
} |
The changes are:
- Extract
f(head)
, which is eagerly evaluated in both branches, as aval
. - Replace the
Option.fold
by a pattern-match. - Replace the cases of
headOption
by two cases on thetail
lazy list itself. For the first case, there seems not to be any case-object, likeNil
, for the empty list. Discussion. - In the second case, abbreviate
(b, acc) => b +: acc
by underscores_ +: _
.
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.
Hello, I see what you mean there.
On my part, usually I try to avoid pattern match (it's slow)
9c6e999
to
fbecfba
Compare
Codecov Report
@@ Coverage Diff @@
## master #3382 +/- ##
==========================================
- Coverage 91.63% 91.60% -0.03%
==========================================
Files 382 382
Lines 8304 8311 +7
Branches 232 226 -6
==========================================
+ Hits 7609 7613 +4
- Misses 695 698 +3 |
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.
Thanks @gagandeepkalra :)
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.
This looks good to me! Do you mind rebasing to resolve the conflicts, @gagandeepkalra? I can merge it after that.
fbecfba
to
dd3c3bc
Compare
6dbb35a
to
50a0cf9
Compare
Thank you @travisbrown @LukaJCB, this was fun. I'm happy knowing short-circuiting is already useful (I see foldable there). |
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.
Thanks a ton!
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.
Thanks again, @gagandeepkalra!
resolves #3381
depends on #3375