selfrefactor / rambda

Faster and smaller alternative to Ramda
https://selfrefactor.github.io/rambda
MIT License
1.65k stars 89 forks source link

fix: cleaner dropWhile/dropLastWhile/takeWhile/takeLastwhile #667

Closed agriffis closed 1 year ago

agriffis commented 1 year ago

There was some sub-optimal code in these functions. I tried to clean it up while keeping your coding style :smile:

agriffis commented 1 year ago

I forgot to run yarn out but I guess you can see what you think

selfrefactor commented 1 year ago

You did some outstanding job today, @agriffis

Thank you for that. I will merge and afterwards, I will look deeper into the problem you are solving.

agriffis commented 1 year ago

@selfrefactor By the way, I'm using these functions locally while waiting for a release:

const dropLastWhile = pred => items => {
  let i = items.length
  while (i && pred(items[i - 1])) {
    i--
  }
  return items.slice(0, i)
}

const takeLastWhile = pred => items => {
  let i = items.length
  while (i && pred(items[i - 1])) {
    i--
  }
  return items.slice(i)
}

I didn't use this approach in my cleanup PR because it seemed like you were avoiding .slice() for some reason. But my guess is that it would be faster than copying with a manual for-loop and .push()? I haven't measured or anything.

For strings you'd need to use .substring() but that's easy enough.

Just thought I'd mention it in case you want to explore these options.

agriffis commented 1 year ago

Following up to my last comment, it looks like .slice() is a LOT faster than for-loop-push, at least on Chrome:

https://www.measurethat.net/Benchmarks/ShowResult/362916

On Firefox they're closer in performance (but for some reason MUCH faster than Chrome, which I didn't expect)

https://www.measurethat.net/Benchmarks/ShowResult/362915

I think the conclusion is that implementations like what I provided above would be a lot more performant than the current approach.

I'll probably make a PR

agriffis commented 1 year ago

Oh, I guess that's just for the copy part. It would still need to iterate to run the predicate.

I need to work on some other things at the moment, but I'll come back to this, run a proper benchmark, and then make a PR if it makes sense.