-
Notifications
You must be signed in to change notification settings - Fork 12k
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
[SUGGESTION] Array, xValues Generalizations #295
Comments
I believe it is not always a viable option to have variadic functions. For example |
@kingdavidmartins shouldn't Maybe instead we should make a generic utility function and link to it form the description of const arrayAdapter = fn => arr => fn( ...arr )
// const maxIn = arrayAdapter(Math.max)
// maxIn([1,2,3]) // -> 3 and possibly one called const variadicAdapter = fn => (...arr) => fn(arr)
// can't think of a use case for this that isn't another library example or specifically composition
// but DANG this would be so useful with flip After all this adapter family is extremely useful with things like |
@skatcat31 I agree that Abstracting to create an array and variadic adapter seems like a very useful and powerful idea for a snippet, especially knowing that this can help a lot of people understand the very features I mentioned above. I'd love to see a PR for that, as it is a really nice snippet, especially because of its simplicity. As for the rest, I am not sure how to proceed with snippets that calculate the |
I think if that were the case the adapters could be good examples. Make me want to do a whole library for small simple adapter functions |
@skatcat31 You could actually start a new category with adapter functions, that would expose a lot of people to them and we can help you maintin it. I love this idea to be honest and I would love to help you out! |
I'll start submitting PRs after work then. Need to get ready to do merges and deploys for work |
Hey @skatcat31 👋 Great Idea, exposing people to adapters will def bring a lot of value to the project. I'm soooo excited and def think a lot people will be able to gain insight from alot of different concepts as the project develops. Especially as they start working with different API/Projects applying said concepts/principles, everything will fall in place and things will start to be/feel more intuitive |
So, now that we are starting to get adapters implemented, where do we stand on the original question that was asked in this issue? |
Hey @Chalarangelo 👋 Yea, I understand Let's say I had a project and wanted to be able to find the max value of collection's. Let's say I then pull a snippet from 30 seconds of code -> The following now happens What we now have is 3 different methods/snippets used/implemented just to do essentially 1 things and that is finding the max value from a collection of sorts. Although this might seem non trival it can get messy and annoying when the same happens with mulitpe different function like lcm, gcd, min, sum, average etc. I believe if people should copy/paste/use our snippet. We should abstract it all into 1 function which still would be easy to understand seeing that all that would be added the said functions would be a line or 2. It begs the question. What's the point of having/pasting 3 different variations of a method that does the samething if it can be one. Especially if the functionality doesn't change. All I have to do just get It's really more about a design decision that would make things much easier and simplier for said users |
First of all, we need to agree that there are 3 distinct categories of snippets that this question applies to:
As far as 3 goes, I think we have already sort of agreed it would be beneficial to extend our snippets to work with both types of input, so I vote we do that. As far as 2 goes, my opinion was and still is that these are useful to expose new concepts to beginners and should be kept as there are no native methods or other snippets. Extending them to variadic input doesn't seem feasible to me, but if it is doable without messing up the code and still keeping it easy to understand for beginners, be my guest. As far as 1 goes, there is little to no reason to keep those or add methods that generalize native functions, as the native ES6 code is definitely better than the one we can create here. Extending |
Is it fair to assume that |
@kingdavidmartins yes (and probably there are more like it), I just don't remember every snippet, these were examples. |
@Chalarangelo Gotcha, just making sure. Yea a few
Honestly after reviewing your response I concur, I believe we are mostly of the same mind/opinion. I also believe updating the snippets that fall in category 2 are doable and could also be beginner friendly. Shouldn't take more than 1 line of code. As such I vote to update As far as 3 is concerned it is possible however it's also improbable that it will be beginner friendly and as such concide Also vote to update |
@kingdavidmartins Alright, let's see what the others have to say then and act accordingly afterwards. |
https://stackoverflow.com/a/47942018/8979804 |
@kriadmin That is quite an elegant solution that should help improve a few snippets, without making them hard to read. I am not against it, if everybody else agrees on this! @skatcat31 @kingdavidmartins Opinions? |
Hey @kriadmin 👋 Thanks for proactively reaching out on SO, sincerely appreciated. This is exactly what I meant by the following lol
const fn = (...arr) => [].concat(...arr) I'm for it 😄 |
Alright, @skatcat31 you are the only person we need an opinion from before we implement these changes. @kriadmin If you want to make a PR after we decide on this, reference the issue and state that these changes are already approved, so that anyone who reviews merges them asap. 😉 |
That's the flatten snippet by another implementation isn't it? I'm all for composition, but if this isn't meant to be a library then the self contained call is the way to go since it will also not need them to bring in another snippet. One snippet, one use. If we want to make it a "improved version" that can also handle multiple arrays(a very common adaption of the function) then this is fine as it makes the status more of an example of an improved adapter without the name. The other solutions are(in theory, did not test because the spread version is much cleaner, and would be the version we want to use) const arrayMax = ( ...arr ) => arr.reduce( ( acc, val ) => Math.max( acc, ...val ), 0 )
const arrayMax = ( ...arr ) => Math.max( arr.reduce( ( acc, val ) => acc.concat(val), [] ) ) however it really is six of one, half dozen of another since the spread operator would achieve the same thing: Looping over the input to concatenate them into a single result set by using I'm all for this since it makes an interior array the focus and thus makes pipe( flatten, spreadOver( Math.max ) ) but that's a library creation and implementation through FP ideologies, and not within the scope of this project. side note: we should update flatten to match this code from SO |
As an aside (because I didn't want to create a new issue) Since you are mentioning
When implementing whatever change you do for these two routines, please check that the |
@tshinnic that should be a new issue, and is a great jumping point for a PR |
Ah, but in one of the PRs implementing Unless, of course, it is somewhere stated a guideline that each snippet must be completely self-sufficient, not using other definitions, and including all dependencies inline? Anyway, just seems intimately tied to naming issues here...? |
It's because each snippet is supposed to be self contained. We aren't a library, but it does raise a different point that should be in a different issue |
@skatcat31 Agreed. Was thinking the same |
I missed a couple of days of this discussion, I think we are drifting away from the original question. We mostly agree on the fact that we should try to make snippets work with either arrays or variadic arguments when possible, remove the ones that just extend native functions by a tiny bit (like Seeing as we also now have adapters, I think we can get away with keeping some functions only variadic or only array-based, as adapters can deal with those relatively complex cases in order for us to focus on the implementations. Oh, and adapters show up first before arrays and other categories, so they should be the first thing anyone sees visiting the project and I sincerely hope people will figure out that they are the way to go when trying to apply a variadic function of ours to an array and vice versa. If we are agreed on these points, let's list the snippets that we want to mark for deletion, improvement and alteration/merge and start working on PRs. Otherwise, comment below and keep the discussion on point. |
@Chalarangelo I totally agree with you. |
I think our original intention can't be changed,so I agree with @Chalarangelo‘s point of view. |
@kriadmin @lvzhenbang reactions keep chains shorted while showing support or descent |
I think we should just have a single variadic snippet. const lcm = (...arr) => {
const gcd = (x, y) => (!y ? x : gcd(y, x % y));
const lcm = (x, y) => x * y / gcd(x, y);
return arr.reduce((a, b) => lcm(a, b));
}; The examples can show how to use it with an array (just spread it -- lots of other snippets use spreading): lcm(1, 2, 3, 4, 5);
lcm(...[1, 2, 3, 4, 5]); This makes it like Math.X functions. |
I agree with @atomiks in that we can showcase |
const lcm = (...arr) =>{
arr = [].concat(...arr)
const gcd = (x, y) => (!y ? x : gcd(y, x % y));
const lcm = (x, y) => x * y / gcd(x, y);
return arr.reduce((a, b) => lcm(a, b))
} Would be better for |
@kriadmin I have no problem with doing something like that, but we should never mutate arguments. Use something like |
@Chalarangelo it's not being mutated there. Also it's scoped to the function, it's not changing the input |
@atomiks ah you are right, my bad! |
I'm confused why we want a multi adapted input? It can lead to more errors. We also have an issue with singular inputs: const lcm = (...arr) =>{
arr = [].concat(...arr)
const gcd = (x, y) => (!y ? x : gcd(y, x % y));
const lcm = (x, y) => x * y / gcd(x, y);
return arr.reduce(lcm)
}
lcm('yo') //"yo" <<< SHOULD BE NaN due to not enough inputs!
lcm('yo', 2) // NaN
lcm('123ac')//"123ac <<< should be NaN
lcm(n)//n <<< should be NaN
lcm('1','2','3')//6, holds due to coercian |
@skatcat31 It can and it does, we might want to revert to simple variadic with better examples then. As far as |
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. |
[SUGGESTION] Array, xValues Generalizations
Description
I believe we should decide how we want methods that take a array or collection of values to be implemented
When going through the snippets I realized that methods like
arrayMax()
orarrayMin()
or any other general method meant for a collection of values only work for 1 specific type.For example:
I think rather than creating a seprate method to handle the said exception/problem I think we could just easily handle and update all snippets with a oneliner that could easily fix the stated problem
I think its important for the following reasons: consistency, readability, performance, and modernity
Early on in the project we decided to stray from implementing
pull()
forpullAll()
simply because 1 takes arr and the other take x amount of values and it didn't makes sense to implement 2 different functions that do the same thing when can easily be turned to 1 function.It also helps reduce clutter
we ended up decided merging it into 1 function.
Now there's quite a few of snippets initializing like so. Only implementing one or the other
Just wanted to make sure I knew which is preferred in the future when implementing/updating other array/collection type methods/snippets & the reasoning behind which we do so choose
The text was updated successfully, but these errors were encountered: