Discussion on: A JavaScript interview question asked at Google

[Costin Manda]

I would say that the implementation is straight forward, but my brain focused on the events member and how it is being used. I have seen this pattern many times and I am surprised there are no common specialized classes for it.

So, first of all, without changing any types, I would optimize the on (and similarly emit) methods like this:

on (eventName, callbackFn) {
  let list = this.events[eventName];
  if (!list) {
    list = [];
    this.events[eventName] = list;
  }
  list.push(callbackFn)
}

It's a minor improvement for small numbers, but it hurts me to see repeated key access to the same reference.

Second of all, why not write first a class that we could call Lookup to handle all those string indexed lists?

class Lookup {
    constructor() {
        this._items=new Map();
    }

    add (key,obj) {
        let list = this._items.get(key);
        if (!list) {
            list=[];
            this._items.set(key,list);
        }
        list.push(obj);
    }

    remove (key, obj) {
        const list = this._items.get(key);
        if (!list) return;
        const index = list.indexOf(obj);
        if (index>=0) {
            list.splice(index,1);
        }
    }

    get(key) {
        // can return an empty array, too, but one could then try to push items into it
        return this._items.get(key) || function*(){};
    }

    clear(key) {
        if (typeof key === 'undefined') {
            this._items.clear();
            return;
        }
        this._items.delete(key);
    }
}

Now, if events is a Lookup class, the code is cleaner and safer:

on (eventName, callbackFn) {
  this.events.add(eventName, callbackFn);
}
emit (eventName, eventData) {
  for (const fn of this.events.get(eventName)) {
    fn(eventData);
  }
}
removeListener (eventName, callbackFn) {
  this.events.remove(eventName, callbackFn);
}

In fact, most of the code in your EventEmitter class is actually Lookup code. One could add to the Lookup class a forEach method and you wouldn't even need an EventEmitter class:

forEach(key,func) {
  for (const item of this.get(key)) {
    func(item);
  }
}

Also, being all modern and not ending statements in semicolon feels obnoxious to me. It's probably me being me and old, but I had to say it.