Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Ensure linear time for the with operation is clear and not a footgun #22

Closed
rricard opened this issue Apr 20, 2021 · 19 comments
Closed
Labels
question Further information is requested
Milestone

Comments

@rricard
Copy link
Member

rricard commented Apr 20, 2021

During April 2021 meeting the concern around the fact that those new methods are o(n) instead of some of their mutator counterparts being o(1) was raised.

It should be clear while using those methods that the cost is o(n). This is partially related to #10.

@rricard rricard added the question Further information is requested label Apr 20, 2021
@rricard rricard added this to the Stage 2 milestone Apr 20, 2021
@rricard rricard self-assigned this May 21, 2021
@rricard
Copy link
Member Author

rricard commented May 21, 2021

#28 might probably evidence a more costly operation and can make this less of an issue. What do people think?

@rricard rricard removed their assignment May 21, 2021
@rricard rricard modified the milestones: Stage 2, Stage 3 May 21, 2021
@theScottyJam
Copy link

I think by nature, people would expect a function that creates a copy of an array to take O(n), after all, each element has to be duplicated somehow. I mean, [...array] is an operation that also takes O(n), but the syntax does not provide any visual indication of this. As long as these functions are able to pass the low bar that's been set by the spread syntax, then I'm good.

@rricard
Copy link
Member Author

rricard commented Nov 10, 2021

Since we reduced the scope of the proposal to remove methods such as pushed or popped this is not an ongoing issue anymore: it is mostly evident that spliced/sorted/... will all be linear time so i'm gonna close that issue and mark it as solved

@rricard rricard closed this as completed Nov 10, 2021
@acutmore
Copy link
Collaborator

acutmore commented Nov 13, 2021

I'm happy for this issue to remain closed, as we not longer have popped and pushed.

Though for completeness we do still have .with to change just one value at a particular index. So this one method could still have the concerns of not being clearly a linear time operation. I have tried to find alternative names for this method that could help clarify it's performance characteristics but without success.

@rricard rricard changed the title Ensure linear time for those operations is clear and not a footgun Ensure linear time for the with operation is clear and not a footgun Nov 15, 2021
@rricard
Copy link
Member Author

rricard commented Nov 15, 2021

Reopening with more precise naming

@rricard rricard reopened this Nov 15, 2021
@acutmore
Copy link
Collaborator

acutmore commented Jan 9, 2022

One way of reducing the linear time with potential performance concern is allowing the method to update multiple indexes at once. To avoid code like the following:

const arr = [1, 2, 3, 4];
const update = arr.with(0, 'a').with(1, 'b').with(3, 'd'); // ['a', 'b', 3, 'c'] - non-efficient use of 2 intermediate copies 

And have a with method that takes an object with multiple changes to make in one pass:

const arr = [1, 2, 3, 4];
const update = arr.with({ 0: 'a', 1: 'b', 3: 'd' }); // ['a', 'b', 3, 'd']

While this makes the method more powerful, it also makes the base case of updating one index a more costly operation, as the object argument needs to be created and inspected (ownKeys). It would also be unclear how conflicting arguments would work:

const arr = [1];
const update = arr.with({ 0: 'a', -1: 'b' }); // ['a'] or ['b'] ?

Overall after the time I've spent thinking about this change I'm not convivned it solves the problem of with potentially not being clear that it is a linear time operation. If someone does want to change multiple indexes in one pass then they could do this with map.

const arr = [1, 2, 3, 4];
const update = arr.map((v, index) => {
  if (index === 0) return 'a';
  if (index === 1) return 'b';
  if (index === 3) return 'd';
  return v;
}); // ['a', 'b', 3, 'd']

@ljharb
Copy link
Member

ljharb commented Jan 9, 2022

you could also do this with Object.assign([1, 2, 3, 4].slice(), { 0: 'a', 1: 'b', 3: 'd' }).

@acutmore
Copy link
Collaborator

acutmore commented Jan 9, 2022

Yep, only downsides to that are wouldn't directly work for tuples, and no implicit relative index. So would have to do:

let t = #[1, 2, 3, 4];
let t2 = #[...Object.assign([t...], { 0: 'a', [t.length - 1]: 'd' })];
t2; // #['a', 2, 3, 'd']

@zloirock
Copy link
Contributor

zloirock commented Jan 9, 2022

@acutmore your option of passing an object to .with looks good. But does it save the current key -> value signature if the first argument is not an object or completely replaces it?

@acutmore
Copy link
Collaborator

acutmore commented Jan 9, 2022

I guess you could overload the method. Off the top of my head I can't think of other array prototype methods that are overloaded though.

@zloirock
Copy link
Contributor

zloirock commented Jan 9, 2022

As an option, it could be a new method like .withAll.

@ljharb
Copy link
Member

ljharb commented Jan 9, 2022

What are the concrete use cases for this?

@zloirock
Copy link
Contributor

zloirock commented Jan 9, 2022

@ljharb have you never had situations when you need to replace some already known array elements?

@ljharb
Copy link
Member

ljharb commented Jan 9, 2022

I've never had a use case where i need to replace more than one element by index. I've had tons of use cases for "replacing multiple elements" (that's what .map is for) and a few for "replacing one element by index" (which is what .with is for).

@zloirock
Copy link
Contributor

zloirock commented Jan 9, 2022

I have cases for a such method regularly, but yes, not very often. At least, something like

array.with({ [i]: array[i + 1], [i + 1]: array[i] })

@ljharb
Copy link
Member

ljharb commented Jan 9, 2022

OK, so, swapping an array's elements at two arbitrary indexes - can you help me understand why you'd want to do that?

@zloirock
Copy link
Contributor

zloirock commented Jan 9, 2022

I even don't know. Maybe because it's too common case of work with collections? For example, changing the order of songs in the playlist or... any other case sorting of an array?

@acutmore
Copy link
Collaborator

Thinking about this again. Maybe there is less concern on the linear time of with, as it is always linear time - and doesn't have a similar method to be confused with. Unlike the concern for pop vs toPopped where there would be two 'pop' based methods; one constant one linear.

@acutmore
Copy link
Collaborator

Closing this issue based on the assessment I had in my last comment above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants