- Notifications
You must be signed in to change notification settings - Fork254
Fixes #426. Add fj.data.NonEmptyList.sequence*, traverse*, foldLeftN, foldRightN, init, last.#427
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:series/4.x
Are you sure you want to change the base?
Conversation
…e*, foldLeftN, foldRightN, init, last.
| finalF<A,B>transform) { | ||
| returntail().isEmpty() ? | ||
| transform.f(head()) : | ||
| init().foldRight(combine,transform.f(last())); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we should be reversing and then folding left,
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For the param and return values, I don't think the textual description adds clarity to the types on the function parameters. Applies elsewhere.
| publicfinal <B>BfoldRightN( | ||
| finalF2<A,B,B>combine, | ||
| finalF<A,B>transform) { | ||
| returntail().isEmpty() ? |
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 and theF<A, F<B, B>> version should be implemented by one calling the other using curry/uncurry.
| finalF<A,B>transform) { | ||
| returntail().isEmpty() ? | ||
| transform.f(head()) : | ||
| init().foldRight(combine,transform.f(last())); |
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.
I think we should be reversing and then folding left,
| returntail; | ||
| } | ||
| publicList<A>init() |
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.
Trivial: Prefer the{ on the same line as the method name. Applies elsewhere.
| * @param combine the combine function | ||
| * @return the output | ||
| */ | ||
| publicfinalAfoldRight1( |
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.
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( | ||
| finalF<A,Either<B,R>>f) { | ||
| returnfoldRightN( | ||
| element ->either ->f.f(element).left().bind(elementInner ->either.left().map(nonEmptyList ->nonEmptyList.cons(elementInner))), |
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 functions and those below use foldRightN, which needs fixing.
| publicclassNonEmptyListTest { | ||
| @Test | ||
| publicvoidtestSequenceEither() { | ||
| assertEquals(right(nel("zero")),sequenceEither(nel(right("zero")))); |
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.
I think using ints, instead of strings would be easier and better.
Fixes#426. Add fj.data.NonEmptyList.sequence*, traverse*, foldLeftN, foldRightN, init, last.