Skip to content

Conversation

@DavidHarrison
Copy link

  • Add comments on the complexity of more functions
  • Reduce the complexity of findLastIndex
  • Generalize replicateM and filterM to Applicative
  • Reuse existing functions where reasonable in definitions

…e monadic functions to `Applicative`, reduce explicit recursion.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this definitely still O(n)? It only is if the singletons are concatenated to the left of the result.

Copy link
Author

Choose a reason for hiding this comment

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

I believe it should be the same as the above functions using foldMap, since foldMap is right fold, and concatenation to the singleton should be constant time. It does feel like a fair amount of wrapping/unwrapping, perhaps it would be better to define mapMaybe (and use it for filter as well)?

@paf31
Copy link
Contributor

paf31 commented Jul 26, 2015

Looks great! I added a couple of comments.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this will be a lot slower than the original definition - reconstructing the entire list and taking an element vs reading it once and taking the last element?

Copy link
Author

Choose a reason for hiding this comment

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

I hadn't been thinking of that. I'll try using a fold.

Copy link
Member

Choose a reason for hiding this comment

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

Currently TCO will optimise this, a fold will be slower too.

@paf31
Copy link
Contributor

paf31 commented Dec 24, 2016

Sorry this has gone unmerged for so long. Would you be interested in rebasing this on top of master? Or maybe this could be broken up into a few smaller PRs.

@DavidHarrison
Copy link
Author

Thanks! I'll split it up into smaller PRs.

@paf31
Copy link
Contributor

paf31 commented Dec 25, 2016

Ok thanks! I'll close this one then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants