The original source code is shown at the bottom of this page.
My comments were as follows…
The sendEvents method stores this.events, then resets this.events to an empty Array but would it not be less confusing to just wait until after sending the data to the server (via XHR) before clearing the Array. For example…
var xmlhttp = null;
if(typeof XDomainRequest != "undefined")
{
xmlhttp = new XDomainRequest();
xmlhttp.open("POST",this.s);
}
else
{
xmlhttp = new XMLHttpRequest();
xmlhttp.open("POST",this.s, true);
xmlhttp.setRequestHeader("Content-type", 'application/json');
}
xmlhttp.onreadystatechange = function()
{
if(xmlhttp.readyState == 4)
{
if(callback)
{
callback(xmlhttp.status);
}
}
}
try
{
xmlhttp.send(JSON.stringify({events:this.events}));
this.events = [];
}catch(e)
{
console.log("caught:"+e);
}
Make the properties on the event object clearer:
this.s = config.server should be this.server = config.server
The date creation isn’t wrapped properly:
(new Date().getTime()) should be (new Date()).getTime()
we’re not sure if the current way will cause an error in certain browsers or not, hence we just felt it would be safer to wrap the new date in parenthesis first before chaining the get time method.
Not sure if the code should be written in the Constructor pattern as there is only ever going to be one instance of the Analytic object so it would be better if it was written as an object literal instead.
Coding style currently matches our PHP guidelines but that could introduce issues with minifiers so it needs to follow our JavaScript style guide
The module returns an object which maps to the Constructor {AC:AC} but it seems redundant to do that. If we do keep with the Constructor pattern then the module should just return the Constructor and not an object literal (unless of course the object that is returned will have additional public API’s added to it which will be consumable).
Again, if we keep with the module returning an object then the AC property should be renamed to something clearer like init:
return {AC:AC}; should be return { init: AC };
Remove the distracting and quite hideous redundant comments: //----------------------------------------------------------------------------- as they make the code much harder to read.
The load method creates a self variable and assigns this to it, but that seems redundant? The only exception is the setTimeout which changes the scope to be window. But for that instance we should be able to get rid of the self variable and instead use the .bind method…
setTimeout(function(){
var data = window.performance.timing;
data.url = window.location.pathname;
this.addSendEvent("PT", data);
}.bind(this), 0);
…or alternatively you could create a helper method to wrap the callback function so it binds this correctly (see: MDN Polyfill)
I’d cache the window reference so the code isn’t constantly doing scope lookups.
Currently all the methods attached to the AC prototype look to be public api methods and really (from a design point of view) I assume these methods should be internalised so they are private and not public.
##Original JavaScript code:
//-----------------------------------------------------------------------------
if (typeof define !== 'function') { var define = require('amdefine')(module)}//node.js magic
//-----------------------------------------------------------------------------
define(function(require){ //BEGIN AMD
//-----------------------------------------------------------------------------
/**
@classdesc The Analytic Client provides an API for applications to submit
events for collation and later analysis.
This version makes some assumptions based on its intended usage.
>=IE9 support only, requires native JSON and XMLHttpRequest or XDomainRequest.
@param {object} config This contains the followin attributes:
server : string : the address of the events server
product : string : uniquely identifies product
probability : number : 0 == never, 1.0 == always trigger non critical events
@class
*/
//-----------------------------------------------------------------------------
function AC(config)
{
this.events = []; //this is where we store events until we send them
this.s = config.server;
this.p = config.product;
this.tprob = config.probability
};
//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
AC.EN_IP = 1; //adds user IP address to event server side
AC.EN_GEOIP = 2; //adds user Location based on IP to event server side
//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
/**
Add an event to the internal queue.
@param {number} eventId The id of the event
@param {Object} eventData The payload data of the event
@param {array} enrich (optional) An array of possible enrichments
*/
//-----------------------------------------------------------------------------
AC.prototype.addEvent = function(eventId,eventData,enrich)
{
this.events.push(
{
p : this.p,
id : eventId,
ts : (new Date().getTime()),
data : eventData,
enrich : enrich
});
};
//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
/**
Add an event to the internal queue.
@param {number} eventId The id of the event
@param {Object} eventData The payload data of the event
@param {array} enrich (optional) An array of possible enrichments
@param {function} callback This will be fired after event has
been added to queue if present
*/
//-----------------------------------------------------------------------------
AC.prototype.addSendEvent = function(eventId,eventData,enrich,callback)
{
this.addEvent(eventId,eventData,enrich);
this.sendEvents(callback);
};
//-----------------------------------------------------------------------------
/**
Create the payload required for the special Page Load event.
This event is automatically triggered whenever the user loads a new
page, it contains detail about their navigation along with performance
timings.
@return {Object} The created Page Load payload.
*/
//-----------------------------------------------------------------------------
AC.prototype.createPLData = function()
{
return {
url : window.location.pathname,
referrer : document.referrer
};
};
//-----------------------------------------------------------------------------
/**
This will send all events currently stored in the client to the
server endpoint.
@param {function} callback Fired upon completion of sending messages.
*/
//-----------------------------------------------------------------------------
AC.prototype.sendEvents = function(callback)
{
var events = this.events;
this.events = [];
var xmlhttp = null;
if(typeof XDomainRequest != "undefined")
{
xmlhttp = new XDomainRequest();
xmlhttp.open("POST",this.s);
}
else
{
xmlhttp = new XMLHttpRequest();
xmlhttp.open("POST",this.s, true);
xmlhttp.setRequestHeader("Content-type", 'application/json');
}
xmlhttp.onreadystatechange = function()
{
if(xmlhttp.readyState == 4)
{
if(callback)
{
callback(xmlhttp.status);
}
}
}
try
{
xmlhttp.send(JSON.stringify({events:events}));
}catch(e)
{
console.log("caught:"+e);
}
};
//-----------------------------------------------------------------------------
/**
When the page has completed loading we create an instance
of the client and submit the special page load event.
*/
//-----------------------------------------------------------------------------
AC.prototype.load = function()
{
var self = this;
if(Math.random() < self.tprob)
{
if(typeof window.performance === 'object')
{
self.addEvent("PL",self.createPLData());
//If called on pageload this will not populate performance data properly
// as the first tick is taken into consideration, thus need to get it on second tick.
setTimeout(function()
{
var data = window.performance.timing;
data.url = window.location.pathname;
self.addSendEvent("PT", data);
},0);
}
else
{
self.addSendEvent("PL",self.createPLData());
}
}
};
//-----------------------------------------------------------------------------
//-----------------------------------------------------------------------------
return {AC:AC};}); //END AMD