kolodny / exercises

Some basic javascript coding challenges and interview questions
4.23k stars 672 forks source link

Incorrect throttle definition #33

Open Wykks opened 9 years ago

Wykks commented 9 years ago

In the test "it will execute every threshold ms", the result expected is [0, 11, 22, 33, 44, 55] But If we strictly respect the definition of throttle (and the name of the test) : throttle is used to make sure the function doesn't execute more than once _per_ x milliseconds It should be [0, 10, 20, 30, 40, 50] right ?

Also, when I run the solution with

var interval = setInterval(throttled, 5);

I get [0,16,28,40,52] ; but it should be [0,10,20,..] ? setTimeout should not take ms but the difference between the last call, in order to call the func as soon as possible.

How I see throttle according to the definition and the test "it will execute at least once more to make up for swallowed calls" : With an interval of 6ms and 10ms for throttle (EDIT: Updated with 6ms) Call 1 (0 ms) direct call ==> [0] +6ms Call 2 (6 ms) --> timed for 4s (not 10ms!) ==> [0] +4ms @10ms Call 2 end ==> [0, 10] +2ms Call 3 (12 ms) --> timed for 8s (not 10ms!) ==> [0, 10] +6ms Call 4 (18 ms) --> clear Call 3 ; timed for 2s ==> [0, 10] +2ms @20ms Call 4 end ==> [0, 10, 20] +4ms .... I know that doing this is not very relevant (at least with such low interval). Maybe I'm just over-thinking the problem, but the definition (or the swallowed calls test) should be more explicit about this behavior.

EDIT 2 : Without this swallowed calls test weirdness, it would be even more simple. And we would get [0,12, 24, 36, 38] as 2 * 6ms > 10ms ; 1 call skipped each time. And it strictly respect the simple need of "limit to one call per x ms".

Thanks for this btw !

darrenscerri commented 9 years ago

I agree, my solution returns [ 0, 10, 20, 30, 40, 50 ] which as per my understanding would by acceptable with respect to the throttle definition that "it will execute every threshold ms". To allow the tests to pass I'm adding 1ms to the ms argument passed to the throttle function.

chemzqm commented 9 years ago

My result changed a lot [ 0, 11, 22, 32, 43, 54 ] [ 0, 12, 22, 32, 42, 52 ] [ 0, 12, 21, 32, 43, 54 ]

PinkyJie commented 7 years ago

I agree, the correct answer should be [ 0, 10, 20, 30, 40, 50 ]. In addition, I think there are 2 more incorrect test cases.

  1. throttle will execute at least once more to make up for swallowed calls:
    • I think the correct answer should be 1.
  2. throttle gets called with the later arguments:
    • I think the correct answer should be [ 11, 22, 33 ]

This article has a fancy animation to illustrate the difference between throttle and debounce.

kolodny commented 7 years ago

I never really gave throttle the proper attention. My first pass at it had the intervals being kicked back into place, for example 10ms kicks at (1,41,63,95,149) => (0, 40, 60, 90, 140), and didn't use mock timers at all. I want to reexamine that approach and also take a closer look at the pending PRs for throttle

mtso commented 7 years ago

I am also getting [ 0, 10, 20, 30, 40, 50 ] with sinon's fake timer.

@PinkyJie The rest of the test cases appear to be accurate, given that:

  1. Calls made to throttle during the throttle "cool down" should be collapsed into a single call made at the end of the throttle, so calling the throttled thunk twice within the threshold should make it call once at the beginning, and then once at the end. The thunk should thus be called twice.
  2. The [33, 44, 55] assigned into args is the result of the above test case behavior.
kolodny commented 7 years ago

This is tricky to fix with fake timers, I've switched to use a "kicker" instead. Let me know if it solves this:

https://github.com/kolodny/exercises/tree/fix-time-issues

sunderls commented 4 years ago

met the same problem,

just a mention that lodash.throttle generates [0,10,20,30,40,50] so I'd say we should stick to lodash