HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

addEventListener - v0

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
addeventlistenerstackoverflowprogramming

Problem

The facade pattern is used to abstract adding events. Should support IE5+ as stands.

Looking for minor improvements. Don't need to support prior to IE5.

I choose not to use a library.

/*addEL
**  dependencies - none 
**  browser - IE5+
**  notes - improved per answer
*/
NS.addEL = (function binding() {
    if (window.addEventListener) {
        return function addEventListener(element, type, callNow) {
            element.addEventListener(type, callNow);
        };
    }
    if (window.attachEvent) {
        return function attachEvent(element, type, callNow) {
            element.attachEvent('on' + type, callNow);
        };
    }
}());

Solution

-
Why does binding have parameters? The closure isn't passing any.

-
As far as I know, 75% of browsers (including IE9+) actually support addEventListener. If you check for attachEvent first, you are wasting that 75% assurance. Instead, check for addEventListener first and 75% of the time, your code is evaluating just one condition.

-
For better readability, use common conventions when naming. Element references can be shortened to el, events are usually ev (because e sometimes refer to error objects), and event handlers are usually called handler or callback.

-
Although you might say that you are not going to support older/other browsers, that does not mean you should recklessly code.

In your code, if you used attachEvent was not supported, you use addEventListener. But what if the browser hasn't got addEventListener either? The binding will crash trying to use it since addEventListener a last resort that is, in this case, not supported.

So I added a third case which makes NS.addEL be a function that does nothing if both methods are not available.

And so:

NS.addEL = (function binding(){

  if(window.addEventListener){
    return function addEventListener(el, ev, handler){
      el.addEventListener(ev, handler);
    }
  }

  if(window.attachEvent) {
    return function attachEvent(el, ev, handler){
      el.attachEvent('on' + ev, handler);
    }
  }

  return function(){/*not supported for some reason*/}

}());


Another approach to this is the following code. It's shorter, but performs checks everytime the function is called. If the browser supports neither, then nothing is actually executed:

NS.addEL = function addEL(el,ev,handler){
  if(window.addEventListener) {
    el.addEventListener(ev,handler);
  } else if(window.attachEvent){
    el.attachEvent('on' + ev,handler);
  }
});

Code Snippets

NS.addEL = (function binding(){

  if(window.addEventListener){
    return function addEventListener(el, ev, handler){
      el.addEventListener(ev, handler);
    }
  }

  if(window.attachEvent) {
    return function attachEvent(el, ev, handler){
      el.attachEvent('on' + ev, handler);
    }
  }

  return function(){/*not supported for some reason*/}

}());
NS.addEL = function addEL(el,ev,handler){
  if(window.addEventListener) {
    el.addEventListener(ev,handler);
  } else if(window.attachEvent){
    el.attachEvent('on' + ev,handler);
  }
});

Context

StackExchange Code Review Q#23282, answer score: 2

Revisions (0)

No revisions yet.