1
\$\begingroup\$

I have a piece of JavaScript that's going to fire event every second after 10s, it's going to gradually increase the interval time. I'm new to JavaScript so it would be nice to have a codereview.

var _lastHeartbeat = window.performance.now();
var _config = {
  heartbeat: {
    start: 1000,
    max: 10000000,
    cap: 10000,
    degrade: function(interval) {
      var int = Math.pow(interval, 1.15);
      return Math.round(int / 10000) * 10000;
    }
  }
}

var heartbeat = function() {
  var end = window.performance.now();
  var timespent = Math.round((end - _lastHeartbeat) / 1000) * 1000
  console.log('test: ' + timespent); // fire event
}

var _startHeartbeat = function() {
  var beat = function(interval) {
    setTimeout(function() {
      heartbeat();
      var end = window.performance.now();
      var timespent = Math.round((end - _lastHeartbeat) / 1000) * 1000
      if (timespent < _config.heartbeat.cap) {
        beat(interval);
      } else if (interval < _config.heartbeat.max) {
        beat(_config.heartbeat.degrade(timespent));
      }
    }, interval);
  };

  beat(_config.heartbeat.start);
}

_startHeartbeat();

Here's JsFiddle. https://jsfiddle.net/noppanit/5cpbyyrg/1/

\$\endgroup\$
1
\$\begingroup\$

Your code looks decent, but I think you might have overthought the problem and the need for so many variables to be globally declared. What you want is a nice function that does everything you want in one single call, and I think it looks something like this:

function heartbeat(delay, interval, duration, callback){
	var now = Date.now();
	var end = now + delay + duration;
	var inc = function(v){ return v; };
	function beat(){
		var now = Date.now();
		if(now < end){
			callback(now, interval);
			interval = inc(interval);
			timeout = setTimeout(beat, interval);
		}
	}
	var timeout = setTimeout(beat, interval);
	var delayTimeout = setTimeout(function(){
		inc = function(v){ return Math.pow(v, 1.15); }
	}, delay);
	return function(){
		clearTimeout(timeout);
		clearTimeout(delayTimeout);
	}
}

You can call it like this:

heartbeat(10000, 1000, 10000000, function(now, interval){
    console.log('beat', interval / 1000);
});

Now for a bit of reasoning why I did this, starting with the variable declarations you have made. The problem with it is that you need about four variables to do this, but setting them as globals means you can have exactly one timer, and calling a new one requires a lot of setup. On top of that, you code is very attached to its execution - you want to separate the implementation from the actual code that uses it as much as possible.

First improvement here is using closures. Since you pass a couple of variables and define the actual bulk of the code inside the function, everything has access to these variables inside the function, but not outside. So your _config and _lastheartbeat variables are not shared by anyone else, and they reference themselves internally so they don't clash. Want to start another heartbeat? No problem, it's variables are contained.

As a last touch, your function needed a return value, something useful, so I decided to return a function that will cancel the timeouts, again thanks to the magic of closures.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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