2
\$\begingroup\$

I'm learning JavaScript and I was trying to make a custom addEvent function that would care about compatibility (I don't want to use jQuery [nor any other library] yet, in order for me to "master" the bases of JavaScript).

I came across this code on github (https://gist.github.com/eduardocereto/955642):

/**
 * Cross Browser helper to addEventListener.
 *
 * @param {HTMLElement} obj The Element to attach event to.
 * @param {string} evt The event that will trigger the binded function.
 * @param {function(event)} fnc The function to bind to the element. 
 * @return {boolean} true if it was successfuly binded.
 */
var cb_addEventListener = function(obj, evt, fnc) {
    // W3C model
    if (obj.addEventListener) {
        obj.addEventListener(evt, fnc, false);
        return true;
    } 
    // Microsoft model
    else if (obj.attachEvent) {
        return obj.attachEvent('on' + evt, fnc);
    }
    // Browser don't support W3C or MSFT model, go on with traditional
    else {
        evt = 'on'+evt;
        if(typeof obj[evt] === 'function'){
            // Object already has a function on traditional
            // Let's wrap it with our own function inside another function
            fnc = (function(f1,f2){
                return function(){
                    f1.apply(this,arguments);
                    f2.apply(this,arguments);
                }
            })(obj[evt], fnc);
        }
        obj[evt] = fnc;
        return true;
    }
    return false;
};

But I was not pleased with the solution even tho it is very short and readable so I made my own (below) with a little help from the book I'm reading: "Secrets of the JavaScript Ninja"

I want to know if you guys think I have something wrong or even if you have any comments or improvements that I might not be seeing:

I commented on that post the following (I'm pg2800 on github):

ORIGINAL COMMENT: 12-January-2014. UPDATED 13-January-2014. I was intrigued with your implementation and I believe I added some best practices to the code in general and also added some improvements to the "traditional" way.

All three implementations of the addEvent custom method below (meaning: with or without any of the addEventListener or attachEvent -- forcing the browser to test all three) worked for: CHROME: Version 32.0.1700.72 m FIREFOX: 26.0 EXPLORER: Version 10.0.9200.16750

Needless to say; I didn't examine all possible scenarios in my test cases, only a few... Let me know what you think.

(function(){
  // I test for features at the beginning of the declaration instead of everytime that we have to add an event.
  if(document.addEventListener) {
    window.addEvent = function (elem, type, handler, useCapture){
      elem.addEventListener(type, handler, !!useCapture);
      return handler; // for removal purposes
    }
    window.removeEvent = function (elem, type, handler, useCapture){
      elem.removeEventListener(type, handler, !!useCapture);
      return true;
    }
  } 
  else if (document.attachEvent) {
    window.addEvent = function (elem, type, handler) {
      type = "on" + type;
      // Bounded the element as the context 
      // Because the attachEvent uses the window object to add the event and we don't want to polute it.
      var boundedHandler = function() {
        return handler.apply(elem, arguments);
      };
      elem.attachEvent(type, boundedHandler);
      return boundedHandler; // for removal purposes
    }
    window.removeEvent = function(elem, type, handler){
      type = "on" + type;
      elem.detachEvent(type, handler);
      return true;
    }
  } 
  else { // FALLBACK ( I did some test for both your code and mine, the tests are at the bottom. )
    // I removed wrapping from your implementation and added closures and memoization.
    // Browser don't support W3C or MSFT model, go on with traditional
    window.addEvent = function(elem, type, handler){
      type = "on" + type;
      // Applying some memoization to save multiple handlers
      elem.memoize = elem.memoize || {};
      // Just in case we haven't memoize the event type yet.
      // This code will be runned just one time.
      if(!elem.memoize[type]){
        elem.memoize[type] = { counter: 1 };
        elem[type] = function(){
          for(key in nameSpace){
            if(nameSpace.hasOwnProperty(key)){
              if(typeof nameSpace[key] == "function"){
                nameSpace[key].apply(this, arguments);
              };
            };
          };
        };
      };
      // Thanks to hoisting we can point to nameSpace variable above.
      // Thanks to closures we are going to be able to access its value when the event is triggered.
      // I used closures for the nameSpace because it improved 44% in performance in my laptop.
      var nameSpace = elem.memoize[type], id = nameSpace.counter++;
      nameSpace[id] = handler;
      // I return the id for us to be able to remove a specific function binded to the event.
      return id;
    };
    window.removeEvent = function(elem, type, handlerID){
      type = "on" + type;
      // I remove the handler with the id
      if(elem.memoize && elem.memoize[type] && elem.memoize[type][handlerID]) elem.memoize[type][handlerID] = undefined;
      return true;
    };

  };
})();

The first two (with addEventListener or attachEvent) run as the original ones, didn't notice any differences. But for the "traditional way":

My original test was 150k repetitions of adding an empty function to the element's event and then run the event. But as you wrap the handlers onto each other; javascript sends the next error: "Maximum call stack size exceeded" which is only natural.

Then I tested for the maximum stack size allowed which was 7816 ( I made that my test size), the results of adding 7816 empty functions to the same type of event of the same element and then executing the event was:

Your code: minimum = 19ms, maximum = 33ms, average = 30ms. My code: minimum = 20ms, maximum = 37ms, average = 27ms.

There is obviously not an improvement on performance whatsoever, but we can now delete specific handlers and also we have room for more handlers, and we can use this to standarize our code with the same function to add and to remove events, so we don't have to worry about X-browser considerations.

If we were to have very little to none memory available, I would definitely go with your implementation.

--> Tests done with a Sony vaio 8GB RAM, core i7 second generation.

i.e.

var div = getElementById("divID"); // returns a div element
var handler = addEvent(div, "click", function(){
 /* do something */
}, false);

/* more code */

removeEvent(div, "click", handler);

P.S. Pardon me if I made any grammatical or orthographic mistakes, English is not my native language

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

Your code is clean and consistent in style and formatting. Good job.

I've noticed two small things that are not problems but rather they were unexpected to me and might trip you coming back to this code in 6 months tie

  1. you end all your code blocks with }; when you don't need to.

    Your code:

    if(something){
        // ...
    };
    

    This will not cause issues but it isn't needed. You only need it for statements1 not code blocks.

    statements:

    var something = { someProp: true, other: 'test' };
    var somethingelse = function () { 
        // ...
    };
    myObject.someMethod();
    

    code blocks:

    if(logicalTest){
        // ...
    }
    
    while(count < 0){
        // ...
    }
    
    function myFunction(){
        // ...
    }
    
  2. elem.memoize[type] = { counter: 1 }; and id = nameSpace.counter++; means there will never be a handler with id 0. I'm not sure that is a problem but I assumed it would start at 0 like all JavaScript lists which are 0-based list. In fact I might actually use a list.

    elem.memoize[type] = [];
    elem[type] = function(){
        for(var i =0; i <= elem.memoize[type].length; i++){
            if(typeof elem.memoize[type][i] == "function"){
                elem.memoize[type][i].apply(this, arguments);
            }
        }
    };
    
    // ...
     var nameSpaceList = elem.memoize[type], id = nameSpaceList.length;
    

1 You don't technically need it for statements there are ways of writing javascript without them but I am personally not a fan.

\$\endgroup\$
0
\$\begingroup\$

As I cannot update the question. I will leave the updated code as a new answer, hope this is not forbidden also.

UPDATEs:

  1. Removed the return true; of the window.removeEvent() functions because I was not validating in any way if the event was being removed.

  2. I listened to James Khoury improvements and I removed the unneeded ; on blocks and added them to statements that didn't have them. Also started the counter in 0 -> elem.memoize[type] = { counter: 0 };

I have used it a lot, and it does not give me any problems whatsoever yet. I hope someone finds it useful someday.

(function(){
  // I test for features at the beginning of the declaration instead of everytime that we have to add an event.
  if(document.addEventListener) {
    window.addEvent = function (elem, type, handler, useCapture){
      elem.addEventListener(type, handler, !!useCapture);
      return handler; // for removal purposes
    };
    window.removeEvent = function (elem, type, handler, useCapture){
      elem.removeEventListener(type, handler, !!useCapture);
    };
  } 
  else if (document.attachEvent) {
    window.addEvent = function (elem, type, handler) {
      type = "on" + type;
      // Bounded the element as the context 
      // Because the attachEvent uses the window object to add the event and we don't want to polute it.
      var boundedHandler = function() {
        return handler.apply(elem, arguments);
      };
      elem.attachEvent(type, boundedHandler);
      return boundedHandler; // for removal purposes
    };
    window.removeEvent = function(elem, type, handler){
      type = "on" + type;
      elem.detachEvent(type, handler);
    };
  } 
  else { // FALLBACK ( I did some test for both your code and mine, the tests are at the bottom. )
    // I removed wrapping from your implementation and added closures and memoization.
    // Browser don't support W3C or MSFT model, go on with traditional
    window.addEvent = function(elem, type, handler){
      type = "on" + type;
      // Applying some memoization to save multiple handlers
      elem.memoize = elem.memoize || {};
      // Just in case we haven't memoize the event type yet.
      // This code will be runned just one time.
      if(!elem.memoize[type]){
        elem.memoize[type] = { counter: 0 };
        elem[type] = function(){
          for(key in nameSpace){
            if(nameSpace.hasOwnProperty(key)){
              if(typeof nameSpace[key] == "function"){
                nameSpace[key].apply(this, arguments);
              }
            }
          }
        }
      }
      // Thanks to hoisting we can point to nameSpace variable above.
      // Thanks to closures we are going to be able to access its value when the event is triggered.
      // I used closures for the nameSpace because it improved 44% in performance in my laptop.
      var nameSpace = elem.memoize[type], id = nameSpace.counter++;
      nameSpace[id] = handler;
      // I return the id for us to be able to remove a specific function binded to the event.
      return id;
    };
    window.removeEvent = function(elem, type, handlerID){
      type = "on" + type;
      // I remove the handler with the id
      if(elem.memoize && elem.memoize[type] && elem.memoize[type][handlerID]) elem.memoize[type][handlerID] = undefined;
    };

  }
})();
\$\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.