I use this Timer
function quite a bit lately for all sorts of stuff,
and would appreciate if someone could review/analyze/criticize/verify
and, of course, suggest things I can do to optimize it for high
frequency execution (mainly animation).
My goal was to replicate ActionScript's counterpart (Timer
class) and its usage simplicity: registering fns for timer's event dispatch, starting/stopping/resetting the timer, etc.
//
//
;( function ( _host_, _aproto ) {
var t = true,
f = false,
nl = null,
timerEvent = {
start : "timer-start",
timer : "timer",
stop : "timer-stop",
end : "timer-end"
},
un, // === undefined
_tm;
// Array.prototype extensions helpers:
// .each() .keep() .gc() .has() .empty() .substitute()
_aproto.each = function ( fn, flgIterBw ) {
var len = this.length,
i;
if ( flgIterBw !== t ) {
for (
i = 0;
i < len;
i++
) {
if ( fn.call( this, i, this[i] ) === f ) break;
}
} else {
for (
i = len;
--i >= 0;
) {
if ( fn.call( this, i, this[i] ) === f ) break;
}
}
return this;
};
_aproto.keep = function ( fn ) {
return this.each(
function ( i, o ) {
( fn.call( this, i, o ) === t )
|| this.splice( i, 1 );
},
t
);
};
_aproto.gc = function () {
var toremove = slc( arguments );
return this.each(
function ( i, o ) {
toremove.has( o )
&& this.splice( i, 1 );
},
t
);
};
_aproto.has = function ( v ) {
return this.indexOf( v ) !== -1;
};
_aproto.empty = function () {
return ( this.length = 0, this );
};
_aproto.substitute = function ( arr ) {
return ( _aproto.push.apply( this.empty(), arr ), this );
};
// helper fns
function isobj( o ) {
return o === Object( o );
}
function isplainobj( o ) {
return Object.prototype.toString.call( o ) === "[object Object]";
}
function isfn( o ) {
return typeof o === "function";
}
function isvalid( o ) {
return o !== un
&& o !== nl
&& ( o === o );
}
function owns( obj, p ) {
return obj.hasOwnProperty( p );
}
// loops objects own properties
// breaks if fn return false
function owneach( obj, fn ) {
if (
isobj( obj )
&& isfn( fn )
) {
for ( var p in obj ) {
if ( owns( obj, p ) ) {
if ( fn.call( obj, p, obj[p] ) === f ) break;
}
}
}
return obj;
}
// attaches set of properties to an object
function rig_props( obj, props ) {
if ( isobj( obj )
&& isplainobj( props )
) {
owneach(
props,
function ( p, v ) {
obj[p] = v;
}
);
}
return obj;
}
function slc( arg, i, j ) {
return Array.prototype.slice.call( arg, i, j );
}
function vacate( obj ) {
for ( var p in obj ) {
owns( Object.prototype, p )
|| ( delete obj[p] );
}
return obj;
}
// 'asyncs' a function
function defer( fn ) {
var args1 = slc( arguments, 1 );
return function () {
var args = args1.concat( slc( arguments ) ),
target = this,
origfn = fn;
setTimeout(
function () {
return origfn.apply( target, args );
}
);
return this;
};
}
// gives an object basic event handling support
// .addListener() .removeListener() .triggerEvent()
function listener( obj ) {
if (
isobj( obj )
) {
var handlers = {};
rig_props(
obj,
{
// registers set of fns for an event 'e'
addListener : function ( e ) {
if (
isvalid( e )
) {
var fnargs =
slc( arguments, 1 )
.keep(
function ( i, o ) {
return isfn( o );
}
);
owns( handlers, e )
&& (
_aproto.push.apply(
handlers[ e ],
fnargs
),
t
)
|| ( handlers[ e ] = slc( fnargs ) );
}
return obj;
},
// removes fns registered for 'e' event
removeListener : function ( e ) {
if (
isvalid( e )
) {
if ( owns( handlers, e ) ) {
var fnargs =
slc( arguments, 1 )
.keep(
function ( i, o ) {
return isfn( o );
}
);
fnargs.length
&& (
_aproto.gc.apply(
handlers[ e ],
fnargs
),
handlers[ e ].length
|| ( delete handlers[ e ] ),
t
)
|| (
handlers[ e ].empty(),
delete handlers[ e ]
);
}
} else {
owneach(
handlers,
function ( evt, fns ) {
fns.empty();
}
);
vacate( handlers );
}
return obj;
},
// runs fns registered for evt 'e'
triggerEvent : function ( e ) {
if (
isvalid( e )
) {
if (
owns( handlers, e )
) {
var fireargs = slc( arguments, 1 );
handlers[ e ]
.each(
function ( k, evhandler ) {
defer( evhandler )
.call(
obj,
{
type : e,
data : fireargs,
target : obj,
handler : evhandler
}
);
}
);
}
}
return obj;
}
}
);
}
return obj;
}
//
// declares Timer factory fn
_tm = function ( delay, repeatCount ) {
return ( function ( delay, fireNTimes ) {
var
// timer obj
host = this,
// timer's private state
// used/manipulated by api bellow
timerState = {
'current-count' : 0,
'delay' : Math.abs( parseFloat( delay ) ) || 1000,
'repeat-count' : Math.abs( parseInt( fireNTimes ) ) || Infinity,
'running' : f,
'interval' : un
},
// arguments provided to timer's .start() method
// used as args for triggered fns
fireargs = [];
// attaches api to timer obj
// .start() .stop() .reset() .currentCount() .delay() .repeatCount() .running() .state()
rig_props(
host,
{
// starts timer event dispatch
// sets provided args as
// parameters to triggered fns
// triggers 'timer-start' event
// and 'timer' events
start : function () {
var startargs;
host.running()
|| (
timerState.running = t,
( startargs = slc( arguments ) ).length
&& fireargs.substitute( startargs ),
host.triggerEvent.apply(
host,
[ timerEvent.start ]
.concat( fireargs )
),
timerState['current-count'] += 1,
host.triggerEvent.apply(
host,
[ timerEvent.timer ]
.concat( fireargs )
),
( timerState['current-count'] === timerState['repeat-count'] )
&& host.reset()
|| ( timerState.interval =
setInterval(
function () {
( timerState['current-count'] < timerState['repeat-count'] )
&& (
timerState['current-count'] += 1,
host.triggerEvent.apply(
host,
[ timerEvent.timer ]
.concat( fireargs )
),
( timerState['current-count'] === timerState['repeat-count'] )
&& host.reset()
);
},
timerState.delay
)
)
);
return host;
},
// pauses triggering timer events
// triggers 'timer-stop' event
stop : function () {
host.running()
&& (
( timerState.interval !== un )
&& (
clearInterval( timerState.interval ),
timerState.interval = un
),
timerState.running = f,
host.triggerEvent.apply(
host,
[ timerEvent.stop ]
.concat( fireargs )
)
);
return host;
},
// nulls timer state
// triggers 'timer-end' event
reset : function () {
( timerState.interval !== un )
&& (
clearInterval( timerState.interval ),
timerState.interval = un
);
timerState.running = f;
timerState["current-count"] = 0;
host.triggerEvent.apply(
host,
[ timerEvent.end ]
.concat( fireargs )
);
return host;
},
// how many times timer fired
currentCount : function () {
return timerState['current-count'];
},
// return timer's fire rate in ms
delay : function () {
return timerState.delay;
},
// how many times timer will fire 'timer' event
repeatCount : function () {
return timerState['repeat-count'];
},
// returns boolean
running : function () {
return timerState.running;
},
// returns timers intrnal state{}
state : function () {
return {
currentCount : timerState['current-count'],
delay : timerState.delay,
repeatCount : timerState['repeat-count'],
running : timerState.running
};
}
}
);
return host;
} ).call( listener( {} ), delay, repeatCount );
};
//
// attaches Timer fn to global scope
_host_.Timer = _tm;
} )( self, Array.prototype );
//
// use:
//
// var
// tm = Timer( 1000/50 ); // set timers fq to 50 times a sec
//
// // register fns for 'timer' event
// tm.addListener(
// "timer",
// function () { console.log( arguments ) },
// doStuff1,
// doStuff2
// );
//
// someElement.onmouseover = function () { tm.start( someElement ); };
// someElement.onmouseout = function () { tm.stop(); };
// someElement.onclick = function () { tm.reset(); };
//
// etc.
//
2 Answers 2
There is room for improvement, in no particular order:
Formatting
for ( i = 0; i < len; i++ )
should really follow normal formatting
for ( i = 0; i < len; i++ )
Formatting: the splitting of conditionals into separate lines is overdone, function
rig_props
is the worst case of overdoing it. You can find a fairly authoritative style guide here.Arrow head coding. If code has the following, then it was done wrong:
} ); } ); } } return obj; } } ); } return obj; }
Naming: please use camelCasing and meaningful names. The code is too hard to follow (
slc
,t
,nl
,un
,_tm
). I understand you are used to it, but if you ever want other people to understand/maintain this, then you need to fix this.Naming: underscores used to indicate private properties/functions. They seem bad form for parameters (
_host_
,_aproto
). See also the Crockford style guide.It defines
.each()
, and you should really look into usingForEach()
instead. Also, look intoreverse()
forflgIterBw
.forEach
can be many times faster than a JS loop.Functions like
_aproto.keep
should have at least a one-liner comment as to what it does.Come to think of it, the code that enhances the array prototype should really be an object on its own, and not stashed away in
Timer
as the code is re-usable. This would follow the principle of Separation of Concerns.All in all, the code is hard to maintain, not evenly commented and it does not seem to consider the advances made in JS 1.6 (
ForEach
, filter, etc.) but it still counts onindexOf()
.Finally, if the code were to be rewritten with the above in mind, it could get more meaningful code reviews because more reviewers could then grok it.
What's with the variable names? len, un, i, fn, t, f? Use the full meaningful names. This is the StopWatch class I use for performance, time measuring.
var StopWatch = function (performance) {
this.startTime = 0;
this.stopTime = 0;
this.running = false;
this.performance = performance === false ? false : !!window.performance;
};
StopWatch.prototype.currentTime = function () {
return this.performance ? window.performance.now() : new Date().getTime();
};
StopWatch.prototype.start = function () {
this.startTime = this.currentTime();
this.running = true;
};
StopWatch.prototype.stop = function () {
this.stopTime = this.currentTime();
this.running = false;
};
StopWatch.prototype.getElapsedMilliseconds = function () {
if (this.running) {
this.stopTime = this.currentTime();
}
return this.stopTime - this.startTime;
};
StopWatch.prototype.getElapsedSeconds = function () {
return this.getElapsedMilliseconds() / 1000;
};
Usage
var stopwatch = new StopWatch();
stopwatch.start();
for (var index = 0; index < 100; index++) {
stopwatch.printElapsed('Instance[' + index + ']');
}
stopwatch.stop();
stopwatch.printElapsed();
Output
Instance[0] [0ms] [0s]
Instance[1] [2.999999967869371ms] [0.002999999967869371s]
Instance[2] [2.999999967869371ms] [0.002999999967869371s]
/* ... */
Instance[99] [10.999999998603016ms] [0.010999999998603016s]
Elapsed: [10.999999998603016ms] [0.010999999998603016s]
-
\$\begingroup\$ The variable names are kind of habit and are enough self explanatory and fast to type in. If that's the single critiq to the code I've pressented, then I'm glad you approve the rest of the code. Btw, that's the slick StopWatch fn you got there, but it's missing a topic's point, where the function registrations part for timer ticks comes in? \$\endgroup\$Nikola Vukovic– Nikola Vukovic2013年09月22日 13:13:51 +00:00Commented Sep 22, 2013 at 13:13
-
\$\begingroup\$ I second the use of descriptive variable names. You're looking to polish your code, not to continue hacking on it, right? The only meaningless variable I use in my code is
i
, and only when used as a loop index. \$\endgroup\$whitehat101– whitehat1012013年10月01日 15:55:59 +00:00Commented Oct 1, 2013 at 15:55