Fixes #426. Add fj.data.NonEmptyList.sequence*, traverse*, foldLeftN, foldRightN, init, last.#427
Conversation
…e*, foldLeftN, foldRightN, init, last.
There was a problem hiding this comment.
I think this is overly inefficient. init is O(n), last is O(n) and foldRight is O(n), so we have overall O(n^2). Compare this to List foldRight, which is O(n) for the reverse and then O(n) for foldLeft, for an overall O(n).
There was a problem hiding this comment.
I think we should be reversing and then folding left,
mperry
left a comment
mperry
left a comment
There was a problem hiding this comment.
Good job, we'll need some updates though before accepting the PR.
| /** | ||
| * Fold the list, from right to left. | ||
| * | ||
| * @param combine the combine function |
There was a problem hiding this comment.
For the param and return values, I don't think the textual description adds clarity to the types on the function parameters. Applies elsewhere.
| public final <B> B foldRightN( | ||
| final F2<A, B, B> combine, | ||
| final F<A, B> transform) { | ||
| return tail().isEmpty() ? |
There was a problem hiding this comment.
This and the F<A, F<B, B>> version should be implemented by one calling the other using curry/uncurry.
| final F<A, B> transform) { | ||
| return tail().isEmpty() ? | ||
| transform.f(head()) : | ||
| init().foldRight(combine, transform.f(last())); |
There was a problem hiding this comment.
I think we should be reversing and then folding left,
| return tail; | ||
| } | ||
|
|
||
| public List<A> init() |
There was a problem hiding this comment.
Trivial: Prefer the { on the same line as the method name. Applies elsewhere.
| * @param combine the combine function | ||
| * @return the output | ||
| */ | ||
| public final A foldRight1( |
There was a problem hiding this comment.
Prefer params on the same line as the method name unless it gets overly long, say >80 chars.
| public <R, B> Either<NonEmptyList<B>, R> traverseEitherLeft( | ||
| final F<A, Either<B, R>> f) { | ||
| return foldRightN( | ||
| element -> either -> f.f(element).left().bind(elementInner -> either.left().map(nonEmptyList -> nonEmptyList.cons(elementInner))), |
There was a problem hiding this comment.
This functions and those below use foldRightN, which needs fixing.
| public class NonEmptyListTest { | ||
| @Test | ||
| public void testSequenceEither() { | ||
| assertEquals(right(nel("zero")), sequenceEither(nel(right("zero")))); |
There was a problem hiding this comment.
I think using ints, instead of strings would be easier and better.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## series/4.x #427 +/- ##
================================================
+ Coverage 52.43% 52.64% +0.20%
- Complexity 2565 2617 +52
================================================
Files 153 153
Lines 9244 9314 +70
Branches 482 481 -1
================================================
+ Hits 4847 4903 +56
- Misses 4244 4256 +12
- Partials 153 155 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
Fixes #426. Add fj.data.NonEmptyList.sequence*, traverse*, foldLeftN, foldRightN, init, last.