A JavaScript interview question asked at Google

That looks like an unnecessarily complex and overengineered solution.

Google will love you.

I liked this problem! Didn't read the solution so I could practice it myself. Here's what I came up with:

class EventEmitter {
  constructor() {
    this.events = {};
  }

  on(event, listener) {
    if (!this.events[event]) {
      this.events[event] = [];
    }
    this.events[event].push(listener);
  }

  emit(event, data) {
    this.events[event].forEach(listener => {
      listener(data);
    });
  }

  removeListener(event, listenerToRemove) {
    // Return if we never registered any listeners for this event
    if (!this.events[event]) return;
    this.events[event] = this.events[event].filter(
      listener => listener !== listenerToRemove
    );
  }
}

Edit: Okay, having read the solution, it seems like we did a lot of things similarly! My removeListener may not be too performant, though, since it replaces the whole array.

Edit2: I believe there's a bug in your removeListener that my code accounted for. If you pass in an event that we never listened to, then attempting to execute this.events[event].indexOf(...) will throw an error because this.events[event] will be undefined.

That's a good question, and the solution is really clear👍

I'm not sure I'd call the emit function a closure, it's not returning a function, and it's not being run outside it's lexical scope (although the callbacks are closures themselves I guess)

There's a couple of things that bother me about this solution, both of them in emit.

First, there's no error handling, so a called function could break the forEach with a throw.

The second is a little more subtle, and I'm not sure if, or how it should be fixed. The object is mutable, and one of the callbacks could change it. Is it canonical to not worry about this? How would you go about fixing it? Deep copy? Freezing? Since it's a parameter, not sure the best way.

while I like the content, Google has not been a good place for developers for a while and we should stop idolizing the "anything related to google must be good" label

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.

how about doing a filter for removing a listener i.e filtering in objects which need not be removed.

Sorry to poop the party, but I hope google asks better questions in interviews for their sake.

PubSub

Very good post. Thanks.

Nice, concise and readable answer to a common JS problem that most of us have ended up implementing in one way or another over the years.

I see a lot of comments related to ts, but the article dowsnt seem to use ts, did it get edited?

Hi, Elisabeth.

I want to share my solution to your series in the Korean community.
Are there any rules I must follow?

You could also have used a Set, assuming that functions are added only one.

Holy shit. I ask the same question in my interviews. Its create a simple EventBus with fire,listen and remove methods 😅