Skip to content
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

[FEATURE] Add partition #512

Merged
merged 2 commits into from Jan 8, 2018
Merged

[FEATURE] Add partition #512

merged 2 commits into from Jan 8, 2018

Conversation

Chalarangelo
Copy link
Owner

Description

Lodash #100 -> https://lodash.com/docs/4.17.4#partition

What does your PR belong to?

  • Website
  • Snippets
  • General / Things regarding the repository (like CI Integration)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking improvement of a snippet)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have checked that the changes are working properly
  • I have checked that there isn't any PR doing the same
  • I have read the CONTRIBUTING document.

Copy link
Contributor

@atomiks atomiks left a comment

Choose a reason for hiding this comment

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

I believe using parens + comma operator can make it one line.

const partition = (arr, fn) =>
  arr.reduce((acc, val, i, arr) => (acc[fn(val, i, arr) ? 0 : 1].push(val), acc), [[],[]]);

@Chalarangelo
Copy link
Owner Author

@atomiks I thought about it when I was writing the snippet, it looks slightly confusing like that, I feel. Plus we have a few snippets with the same kind of style, so I decided to go with that style for consistency.

@atomiks
Copy link
Contributor

atomiks commented Jan 8, 2018

@Chalarangelo see the edit- you can also stick the ternary inside the acc[], which makes it shorter. You can also use concat here, no?

nvm concat can't be used since it relies on mutating the inner arrays

@Chalarangelo
Copy link
Owner Author

Array.concat() is terriby slow, so we avoid using it for such cases in general. Sticking the ternary inside the [] is not a bad idea, however the comma operator next to that will just be a bit too much and the rest of the snippets don't use it anyways, so I guess just the ternary should be enough of an improvement.

@skatcat31
Copy link
Contributor

Reduce is a weird monster, but this works as advertised. To only awkward part is needing the parens, which harms readability but not enough to not consider it

@Chalarangelo Chalarangelo merged commit 4fb1f9b into master Jan 8, 2018
@Chalarangelo Chalarangelo deleted the partition branch January 8, 2018 17:58
@lock
Copy link

lock bot commented Dec 19, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for any follow-up tasks.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants