AnalyticsClient Feedback

analyticsclient_feedback.md

1. Can't remember the fancy name of these coding styles, but instead of:

   return function()
   {
       
   if(expression)
   {
   

   you should be using:

   return function() {
    
   if(expression) {
   

   This helps the compiler when we start minifying the code.

2. Remove the doubleDelegate method on AnalyticsClient. It should be up to the client code using the AnalyticsClient on how to initialise the object.

3. Minor code consistency issues. E.g. some function parameters are specified with spaces: function(param1, param2) and some without: function(param1,param2).

4. Remove setTimeout() on line 142 as it seems unnecessary (unless he can clarify?).

5. Remove location.reload() on line 148. This may just have been for debug purposes (?) but ultimately it's going to have a negative affect on the user experience.

6. If the server to which the client sends events to will change on a per product basis, then I would suggest passing the server host through to AnalyticsClient as a constructor parameter:

   AC = new AnalyticsClient('http://myhost.com');
   

7. Make sure you pass the code through a minifier to remove the code comments.

8. The object has been designed using a constructor and then adding a load of instance methods such as addEvent and createPageLoadData to the prototype. This gives the impression that the AnalyticsClient has been designed to be instantiated multiple times. If this is not the case (which I assume it isn't) I would simplify the object by making it an object literal:

   var AnalyticsClient = {
   	addEvent: function() {},
   	createPageLoadData: function() {}
   	...
   };
   

   (Maybe look into using the module pattern to separate his public and private api.)

9. Just a heads up…the window.performance api is fairly recent and the comments about this object providing support for IE8+ is slightly misleading (it certainly isn't available in IE8!).

10. The success callback on line 147 should probably be passed in to the object at runtime to allow for maximum flexibility.

11. On line 34, I'd change: new Date().getTime() to (new Date()).getTime() just to be safe.

12. Code comment on line 41 seems redundant.

13. Line 34 makes it hard to read the keys/values on the object he's pushing onto this.events. For clarity, expand it onto multiple lines.

14. If there's only ever going to be one event pushed on to the this.events array before it's sent to the server, you could clean up lines 91-92 by using var events = this.events.pop(); instead. (Alternatively, why use an array at all?)

15. AC_PRODUCT on line 1 is floating around in global space. Either remove the variable on line 34 and reference "t1" directly, or pass the variable in at runtime to the constructor. Alternatively, internalise the property onto the object if using a literal as suggested in (8.)