3
\$\begingroup\$

I'm trying to create a good way to manage throttling and debouncing a lot of functions. What I have allows you to group, add and remove functions to instances of Booster. Each instance runs it's own timers and can be manipulated by methods.

Are there any major performance concerns with the approach below? I'm brand new to ES6, scope/context issues, and come from a mostly jQuery background—so I don't know much about performance, memory leaks, etc. Any tips are appreciated.

// booster.js

export default class Booster {

  constructor(method, type, amount, ...fn) {
    this.view = window;
    this.type = type;
    this.amount = amount;
    this.fn = fn ? fn : [];
    this.paused = false;
    this.method = method;
    this.init();
  }

  raf() {
    if (this.view.requestAnimationFrame) {
      this.view.requestAnimationFrame(this.run.bind(this));
    } else {
      this.run().bind(this);
    }
  }

  run() {
    if (0 < this.fn.length) {
      this.fn.forEach(function(fn) {
        fn();
      });
    }
  }

  throttle() {
    this.active = true;
    this.view.addEventListener(this.type, () => {
      if (this.active && !this.paused) {
        this.active = false;
        this.raf();
        this.timeout = setTimeout( () => {
          this.active = true;
        }, this.amount);
      }
    }, false);
  }

  debounce() {
    this.view.addEventListener(this.type, () => {
      if (!this.paused) {
        clearTimeout(this.timeout);
        this.timeout = setTimeout( () => {
          this.raf();
        }, this.amount);
      }
    }, false);
  }

  add(...fn) { this.fn.push(...fn); }
  remove(n) {
    if (n < this.fn.length && n >= 0) {
      this.fn.splice(n, 1)
    } else {
      throw new Error('There are only ' + this.fn.length + ' functions in this Booster.');
    }
  }

  pause()    { this.paused = true;}
  resume()   { this.paused = false; }
  toggle()   { this.paused = !this.paused; }

  init() {
    if      ('throttle' === this.method) { this.throttle(); }
    else if ('debounce' === this.method) { this.debounce(); }
    else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
  }
}

usage:

// main.js

import Booster from './classes/booster.js';

let scrollDebouncer1 = new Booster('debounce', 'scroll', 100, myFunction());

scrollDebouncer1.add(myOtherFunction());

scrollDebouncer1.pause();

// etc.
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Problems

  • There was a bug in Booster.raf in the else block you have this.run().bind(this) and I assume you mean this.run.bind(this)() though the binding is not needed because you are in the context of this so this.run() will do just fine.

  • You throw in two places yet ignore many problems that can be introduced by not vetting at the interface. There should be more checking when setting the Booster state. Eg amount should be a Number, should not be less than 0. Booster.remove(n) n could be a fraction, you should ignore or truncate floats.

  • It is possible for a function to execute even after Booster is paused. This will happen if pause() is called after the event is triggered. The run function should make sure that you do not run in paused state.

  • If any of the given functions throw an error then all remaining function on the fn array will not be executed. Nor do you check if any of the functions given are actually functions. This will also prevent functions after the bad function from being called.

  • You do not check if Booster.view has the function addEventListener. In Booster.throttle this will result in the Booster object incorrectly setting the active state to true.

  • It is possible to have more than one event listener if Booster.init is called more than once. Because you only have one Booster.timeout property the Booster.throttle function can overwrite the Booster.debounce timeout property preventing debounce from working correctly (and debounce can stop throttle timeouts breaking throttling)

Design

The logic is not fully thought out. It is always important to test your code for behaviour that are not expected.

I can not tell if you expect init to be called again after construction. The fact that it is outside the constructor suggests so. If this is the case, why are you not providing a way to remove listeners. Calling init will just keep adding listeners and because the added listeners are anonymous there is no way to remove them.

I do not know how many events you are expecting to intercept or delay, but any more than a few and you would be much better of running a continuous rAF loop and handling all the needed events in that one function.

Review by comments.

All comments within /* ... */ are added as I went over the code, and other comments are me modifying your code by removal. The comments are generally unrelated suggestion and were created before I wrote the review above.

export default class Booster {

  constructor(method, type, amount, ...fnArray) {  
    this.view = window;
    this.type = type;     /* type of what? bad naming maybe eventType */
    this.amount = amount; /* amount? bad naming maybe bebounceDelay */
    /* fn will always be an array as you have defined it as so in the arguments
       this.fn = fn ? fn : [];  // bad naming use plural or indicate its an array
       Use existing methods */
    // this.fnArray = fnArray;
    this.add(fnArray);
    /* Use existing methods */
    //this.paused = false;
    this.pause();
    this.method = method; /* method? bad naming maybe methodType */
    this.runBound = this.run.bind(this);  /* Only need to bind once */
    
    /* calling init in the constructor always seams such a silly thing to
        do especially when init does not actually init the class */
    /* Use a setter to set the method */
    // this.init();
  }
  /* Use a setter to set method and remove init as it is not needed */
  set method(type) {
    if      (type === 'throttle') { this.throttle(); }
    else if (type === 'debounce') { this.debounce(); }
    /* Why throw, You are not in control of the calling function so how do you know it
        will be caught. Throwing will stop executing code and in production if 
       not caught end up in a non functioning page.
       Degrade gracefully, don't fall in a heap at the slightest fault. */
    else  { console.warn("Invalid Booster method. Must be \"throttle\" or \"debounce\".") }
    //  else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
  }
  raf() {
    if (this.view.requestAnimationFrame) {
      this.view.requestAnimationFrame(this.runBound);
    } else {
      /* Run then bind ?????   you had this.run().bind(this);
         maybe you meant this.run.bind(this)(); */
      this.run();
    }
  }

  run() {
     /* Don't need to check if zero. For each will do that for you */
    // if (0 < this.fn.length) { 
    /* try to avoid callback iterators as they are slower than for loops */
      //this.fn.forEach(function(fn) {
      //  fn();
      //});
    /* a better way to iterate 
    for(const fn of this.fnArray){ fn() }  */
  }

  throttle() {
    this.active = true;
    this.view.addEventListener(this.type, () => {
      if (this.active && !this.paused) {
        this.active = false;
        this.raf();
        this.timeout = setTimeout( () => {
          this.active = true;
        }, this.amount);
      }
    }, false);
  }

  debounce() {
    this.view.addEventListener(this.type, () => {
      if (!this.paused) {
        clearTimeout(this.timeout);
        this.timeout = setTimeout( () => {
          this.raf();
        }, this.amount);
      }
    }, false);
  }

  add(...fnArray) { this.fnArray.push(...fnArray); }
  remove(n) {
    if (n >= 0 && n < this.fn.length) {
      this.fn.splice(n, 1)
    }
    /* Again you are not in control of calling code and can not know 
      if this will be caught. I dont think this is even worth a warning  */
  //    else {
  //  throw new Error('There are only ' + this.fn.length + ' functions in this Booster.');
    //}
  }

  pause()    { this.paused = true;}
  resume()   { this.paused = false; }
  toggle()   { this.paused = !this.paused; }

  /* removed as not needed, see setter set method */
  //init() {
  //  if      ('throttle' === this.method) { this.throttle(); }
  //  else if ('debounce' === this.method) { this.debounce(); }
  //  else { throw new Error('Invalid Booster method. Must be "throttle" or "debounce".'); }
  //}
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.