Skip to content

Optimise NonEmptyTraverse implementation#3382

Merged
travisbrown merged 4 commits into
typelevel:masterfrom
gagandeepkalra:test/nonEmptyTraverse/short-circuiting
Jun 4, 2020
Merged

Optimise NonEmptyTraverse implementation#3382
travisbrown merged 4 commits into
typelevel:masterfrom
gagandeepkalra:test/nonEmptyTraverse/short-circuiting

Conversation

@gagandeepkalra
Copy link
Copy Markdown
Contributor

resolves #3381

depends on #3375

Comment on lines +463 to 480
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)
}
Copy link
Copy Markdown
Contributor

@diesalbla diesalbla Apr 19, 2020

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:

Suggested change
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 a val.
  • Replace the Option.fold by a pattern-match.
  • Replace the cases of headOption by two cases on the tail lazy list itself. For the first case, there seems not to be any case-object, like Nil, for the empty list. Discussion.
  • In the second case, abbreviate (b, acc) => b +: acc by underscores _ +: _.

Copy link
Copy Markdown
Contributor Author

@gagandeepkalra gagandeepkalra Jun 2, 2020

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)

@gagandeepkalra gagandeepkalra force-pushed the test/nonEmptyTraverse/short-circuiting branch from 9c6e999 to fbecfba Compare May 26, 2020 01:55
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 26, 2020

Codecov Report

Merging #3382 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            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     

LukaJCB
LukaJCB previously approved these changes May 26, 2020
Copy link
Copy Markdown
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gagandeepkalra :)

@travisbrown travisbrown self-requested a review June 2, 2020 09:27
travisbrown
travisbrown previously approved these changes Jun 2, 2020
Copy link
Copy Markdown
Contributor

@travisbrown travisbrown left a 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.

@gagandeepkalra gagandeepkalra dismissed stale reviews from travisbrown and LukaJCB via dd3c3bc June 2, 2020 12:18
@gagandeepkalra gagandeepkalra force-pushed the test/nonEmptyTraverse/short-circuiting branch from fbecfba to dd3c3bc Compare June 2, 2020 12:18
@gagandeepkalra gagandeepkalra force-pushed the test/nonEmptyTraverse/short-circuiting branch from 6dbb35a to 50a0cf9 Compare June 2, 2020 12:28
@gagandeepkalra
Copy link
Copy Markdown
Contributor Author

Thank you @travisbrown @LukaJCB, this was fun. I'm happy knowing short-circuiting is already useful (I see foldable there).

Copy link
Copy Markdown
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton!

Copy link
Copy Markdown
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, @gagandeepkalra!

@travisbrown travisbrown merged commit 0ade708 into typelevel:master Jun 4, 2020
@gagandeepkalra gagandeepkalra deleted the test/nonEmptyTraverse/short-circuiting branch June 4, 2020 13:57
@travisbrown travisbrown added this to the 2.2.0-M3 milestone Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimise NonEmptyTraverse implementation

5 participants