2
\$\begingroup\$

I need to create this eventEmitter class with the functions listed below. I think I could clean the code a little but don't really know where to start.

var eventEmitter =function  (){
  this.listeners = 0;
  this.events = {};
  return this;
};


eventEmitter.prototype.on = function(ev, cb) {
  if (typeof ev !== 'string') throw new TypeError("Event should be type string", "index.js", 6);
  if (typeof cb !== 'function' || cb === null || cb === undefined) throw new TypeError("callback should be type function", "index.js", 7);

  if (this.events[ev]){
    this.events[ev].push(cb);
  } else {
    this.events[ev] = [cb];
  }

  this.listeners ++;
  return this;
};

eventEmitter.prototype.emit = function(eventType) {

  if (typeof eventType !== 'string') throw new TypeError("Event type should be type string", "index.js", 6);
  var handlerFunctions = this.events[eventType];

  if (handlerFunctions) {
    var self = this;

    for (var i = 0; i < handlerFunctions.length; i++) {
      var handler = handlerFunctions[i];

      if (arguments.length > 0) {
        var args = Array.prototype.slice.call(arguments).slice(1, arguments.length);
        handler.apply(self, args);
      } else{
        handler.call(self);
      }
    }
  }
  return this;
};

eventEmitter.prototype.off = function(eventType, handlers) {

  if ( arguments.length > 0 && (eventType === 'undefined' || typeof eventType !== 'string')) throw TypeError('listener must be a function');

  if (arguments.length > 1) {
    if ( typeof handlers !== 'function' || handlers === 'undefined') throw TypeError('handler must be a function string or object');
  }


  switch(arguments.length) {
    case 0:
      this.listeners = 0;
      this.events = {};
      break;
    case 1:
      if (this.events[eventType]) {
        this.listeners = this.listeners - this.events[eventType].length;
        delete this.events[eventType];
      }
      break;

    case 2:

      if (this.events[eventType]) {
        for (var i = 0; i < this.events[eventType].length; i++) {
           if (handlers.toString() == this.events[eventType][i].toString()){
             this.events[eventType].splice( i, 1 );
             this.listeners --;
             i --;
           }
        }
      }
      break;
  }
  return this;
};

module.exports = eventEmitter;
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$
  • Constructors should be PascalCase, i.e. EventEmitter - not eventEmitter. Of course, EventEmitter is the exact name of Node's own implementation, so I'd pick something else.

  • Don't hardcode file and line number in errors. Those get added automatically, which is the whole point. And besides, if you hardcode them, you'll have to keep them up to date (line numbers are already misleading), etc.. It's interpreter-generated metadata, not data.

  • Also, this a minor thing but don't use "should" in an error description. You're throwing an error because, something must be something - not just because it ought to. So event names must be strings, and listeners must be functions.

  • Don't bother with maintaining the listeners count manually. You can just do this (ES6 syntax, but you can translate it):

    Object.keys(this.events).reduce((count, key) => count + this.events[key].length, 0);
    // => number of listener functions
    

    You can add that as a getter function with Object.defineProperty or just have a getListenerCount method. If you add it as a method you can even choose to only get the count for a named event, i.e. getListenerCount('someEventName').

  • This:

    if (typeof cb !== 'function' || cb === null || cb === undefined)
    

    is very redundant. The only thing that matters is whether cb is a function. Doesn't matter if it's null or undefined - it's still not a function.

  • You don't need to return this if you intend to use a function as a constructor. You can, but you don't need to.

Here's a quick refactoring based on the points above:

function EventBase() {
  this.events = {};
};

EventBase.prototype = {
  on: function (event, listener) {
    if (typeof event !== 'string') throw new TypeError("Event must be a string");
    if (typeof event !== 'string') throw new TypeError("Listener must be a function");
    this.events[event] || (this.events[event] = []);
    this.events[event].push(listener);
  },

  off: function (event, listener) {
    if (arguments.length === 0) {
      // remove all listeners
      this.events = {};
      return;
    }

    if (!this.events[event]) {
      // return if there's no event by the given name
      return;
    }

    if (arguments.length === 1) {
      // remove all listeners for the given event
      delete this.events[event];
      return;
    }

    // remove specific listener
    this.events[event] = this.events[event].filter(function (func) {
      return func !== listener;
    });
  },

  emit: function (event) {
    if (!this.events[event]) {
      // return if there's no event by the given name
      return;
    }

    // get args
    var args = [].slice.call(arguments, 1);

    // invoke listeners
    this.events[event].forEach(listener => listener.apply(this, args));
  },

  getListenerCount: function (event) {
    // get total number of listeners
    if (arguments.length === 0) {
      return Object.keys(this.events).reduce((count, key) => count + this.getListenerCount(key), 0);
    }

    // return zero for non-existing events
    if (!this.events[event]) {
      return 0;
    }

    // return count for specific event
    return this.events[event].length;
  }
};
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.