DEV Community

A JavaScript interview question asked at Google

elisabethgross on January 29, 2020

Hello and welcome back to Code Review, a series of coding interview challenges and career related content released weekly exclusively on Dev.to. I’...

Read full post

Brian • Jan 29 '20 • Edited

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)

elisabethgross • Jan 30 '20

Not the emit function - the event callbacks themself close over the data object and gain access to that themselves!

Comment deleted

elisabethgross • Jan 30 '20

Regardless of whatever you two decide is a better answer, its always nice to see other ways of solving the same problem! Play nicely :)

Theofanis Despoudis • Jan 30 '20

That's not so different than a typical implementation:

github.com/Olical/EventEmitter/blo...

Kudos

elisabethgross • Jan 30 '20

Good point that will definitely be a bug! Nice catch!

Robert Myers • Jan 31 '20

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.

Brian • Jan 31 '20 • Edited

Making the eventData immutable is probably not the job of the EventEmitter

The Node.js EventEmitter doesn't alter the data in its emit implementation

Robert Myers • Feb 1 '20

That's why I don't know the solution to this. In C++, passing along const references solves it from anything but something very intentional. If it's not immutable (or rather, some callback decides to mutate it) you're going to run into all sorts of potential bugs. Things can change based on the order of the callbacks and on the implementation of the emitter. That just really grates on me and feels wrong.

I wonder if that's part of the point of the question. When I did C++ interviews, one of my standard questions was "implement memcpy". I was more interested in how they handled edge cases than the actual copying.

José Muñoz • Jan 29 '20

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

elisabethgross • Jan 30 '20

Couldn't agree more. Idolizing any company is a recipe for disappointment. However, they are KNOWN for the caliber of interview questions they ask so its really nice to see some real questions they've asked in the past!

Costin Manda • Feb 2 '20 • Edited

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.

Avichay Eyal • Jan 30 '20

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

elisabethgross • Jan 30 '20 • Edited

Anyone smell that..? ;) Sometimes you get lucky with an "easier" problem - not the worst thing in the world!

Anurag Hazra • Jan 31 '20

PubSub

Christian Melgarejo • Jan 31 '20

The bases for p/s, yes

Sunit Katkar • Jan 30 '20

Very good post. Thanks.

Pavan Maheshwari • Jan 30 '20

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

Matei Adriel • Feb 22 '20

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

Lucas • Feb 3 '20

Hi, Elisabeth.

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

Ali Jalali • Dec 18 '23

nice

Vincent Cantin • Feb 1 '20

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