[SUGGESTION] Array, xValues Generalizations

[kingdavidmartins]

[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() or arrayMin() or any other general method meant for a collection of values only work for 1 specific type.

For example:

const arrayMax = arr => Math.max(...arr);
arrayMax([10, 1, 5, 23, 2, 12445, 25])// -> 12445
arrayMax(10, 1, 5, 23, 2, 12445, 25)// -> Breaks & Throws an Err

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() for pullAll() 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

[Chalarangelo]

I believe it is not always a viable option to have variadic functions. For example Math.max() is a native method, so arrayMax has to be implemented separately. Certain other methods might not fit this case. It's also reasonably easier to understand sometimes, so I am uncertain which is the right decision. Opinions needed on this one!

[skatcat31]

@kingdavidmartins shouldn't arrayMax break for a variadic input since it isn't an array? arrayMax is more an adapter function for collections to use Math.max if they comply to the spread operator(Set, Array, Map, weakMap)[ wait do Map and weakMap work with spread? I may be wrong there ]. Math.max is designed to work only with variadic input by design, and Function.prototype.apply used to be the spec for using Arrays with variadic functions versus today's spread operator way back when.

---

Maybe instead we should make a generic utility function and link to it form the description of adapters and put it in Utility called arrayAdapter:

const arrayAdapter = fn => arr => fn( ...arr )
// const maxIn = arrayAdapter(Math.max)
// maxIn([1,2,3]) // -> 3

and possibly one called variadicAdapter:

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 Promisification, flip, context, bind, etc. but that starts getting into the advanced world of composition and types which I'm also not certain our collection would want to touch without possibly splitting off into a full library, and Ramda has us extremely beet there.

[Chalarangelo]

@skatcat31 I agree that arrayMax shouldn't work for variadic input, after all Math.max() works for variadic input, so that would beat the point of having the snippet to begin with. Snippets like that one are not so much great examples, but more of a way to introduce people to the way you can use array decomposition and the powers of ES6 and variadic functions. Based on that, I am voting to keep at least arrayMax and arrayMin as they expose useful features to new developers.

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 gcd or lcm or other things in an array. But I do like the sum and average ones, as they are quite nice and simple, exposing developers to useful features. So, for me, it boils down to the question: do we keep the gcd and lcm ones?, as they are the only ones that seem a tiny bit off compared to the others.

[skatcat31]

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

[Chalarangelo]

@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!

[skatcat31]

I'll start submitting PRs after work then. Need to get ready to do merges and deploys for work

[kingdavidmartins]

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

[Chalarangelo]

So, now that we are starting to get adapters implemented, where do we stand on the original question that was asked in this issue?

[kingdavidmartins]

Hey @Chalarangelo 👋

Yea, I understand Math.max() by design is meant to work with variadic arguments. and understand why implementing it for just arrays seems like the thing to do. However when thinking about using said snippets I also realized the conundrum.

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 -> arrayMax.md just so I can deal with finding the max value in said array. Now let's say I want to find the max value in a JS object. I also pull that code from Stackoverflow or w.e.

The following now happens
Everytime I come across a array I use -> arrayMax()
Everytime I come across variadic input I use -> Math.max()
Everytime I come across a JS Object -> objectMax()

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 max whenever I come across any sort of collection of values if I want to find the greatest value.

It's really more about a design decision that would make things much easier and simplier for said users

[Chalarangelo]

First of all, we need to agree that there are 3 distinct categories of snippets that this question applies to:

1. arrayMax and arrayMin which rely on native functions and just show off array destructuring.
2. arraySum and arrayAverage which do something specific and do not rely on anything, except clever use of array methods (not really super clever, but a good example to introduce people to the ideas).
3. arrayGcd and arrayLcm which rely on some of our own functions and are extended the functionality.

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 Math.max() for an array is just a simple destructure and extending for a collection is a significantly more complex process that should be very carefully crafted. If we are to do anything, I would suggest removing arrayMax and arrayMin and actually creating similar functions for objects and collections of objects that would be beneficial to more people.

[kingdavidmartins]

Is it fair to assume that median also falls into category 3?

[Chalarangelo]

@kingdavidmartins yes (and probably there are more like it), I just don't remember every snippet, these were examples.

[kingdavidmartins]

@Chalarangelo Gotcha, just making sure. Yea a few

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.

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 arraySum & arrayAverage & Ofcourse if we decide to go for it, I wouldn't mind updating

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 arrayGcd and arrayLcm & wouldn't mind updating.

[Chalarangelo]

@kingdavidmartins Alright, let's see what the others have to say then and act accordingly afterwards.

[rohitanwar]

https://stackoverflow.com/a/47942018/8979804
How about this?
After reading your conversation I decided to ask it on SO and got this answer.

[Chalarangelo]

@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?

[kingdavidmartins]

Hey @kriadmin 👋

Thanks for proactively reaching out on SO, sincerely appreciated.

This is exactly what I meant by the following lol

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.

const fn = (...arr) => [].concat(...arr)

I'm for it 😄

[Chalarangelo]

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. 😉

[skatcat31]

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 flatten.

I'm all for this since it makes an interior array the focus and thus makes arrayMax the true meaning. It just will need an updated description to note that it is flattening the inputs. This way they might also be curious to see if they could just use

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

[tshinnic]

As an aside (because I didn't want to create a new issue)

Since you are mentioning arrayLcm() and lcm() I'd like to note there is already 'drift' between the implementations.

lcm() is correctly defined (from my casual research) in using Math.abs() e.g.

const lcm = (x,y) => {
  const gcd = (x, y) => !y ? x : gcd(y, x % y);
  return Math.abs(x*y)/(gcd(x,y));
};

arrayLcm() is perhaps less correctly defined, as it skips the Math.abs() e.g.

const arrayLcm = 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));
}

When implementing whatever change you do for these two routines, please check that the Math.abs() is used?

[skatcat31]

@tshinnic that should be a new issue, and is a great jumping point for a PR

[tshinnic]

Ah, but in one of the PRs implementing lcm() / arrayLcm() it specifically mentioned not polluting the name space - having a single version of lcm() - and so put a separate implementation inline. Yet, having a single implementation of lcm() would have prevented this problem (lack of Math.abs()). So until the naming is straightened out there would have to be at least two PRs, one fixing the wrong inline definition, and the other one substituting the other already defined lcm().

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...?

[skatcat31]

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

[kingdavidmartins]

side note: we should update flatten to match this code from SO

@skatcat31 Agreed. Was thinking the same

[Chalarangelo]

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 arrayMax) and keep others that are examples that only specifically apply to arrays.

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.

[rohitanwar]

@Chalarangelo I totally agree with you.

[lvzhenbang]

I think our original intention can't be changed，so I agree with @Chalarangelo‘s point of view.

[skatcat31]

@kriadmin @lvzhenbang reactions keep chains shorted while showing support or descent

[atomiks]

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.

[Chalarangelo]

I agree with @atomiks in that we can showcase lcm and gcd working with arrays and remove the arrayLcm and arrayGcd snippets. That should be PRed asap to get these snippets cleaned up and updated. Anyone against this?

[rohitanwar]

@Chalarangelo @atomiks

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 lcm([1,2,3],[5,6,7])

[Chalarangelo]

@kriadmin I have no problem with doing something like that, but we should never mutate arguments. Use something like let data = [].concat(...arr) instead and this should be fine (also update the return statement accordingly). I wouldn't mind merging this, as it can work with more types of input, even if a developer is not familiar with spreading and it's just one line longer (and a pretty easy line to read).

[atomiks]

@Chalarangelo it's not being mutated there. Also it's scoped to the function, it's not changing the input

[Chalarangelo]

@atomiks ah you are right, my bad!

[skatcat31]

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

[Chalarangelo]

@skatcat31 It can and it does, we might want to revert to simple variadic with better examples then.

As far as lcm's test cases are concerned, we usually aren't handling errors or bad input, but maybe this could be added into some methods to preserve the logic of them and the sanity of people using them.

[lock]

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.