Javascript Pub/Sub

Question

Here is a short pice of code that suppose to work as Pub/Sub or EventDisptacher of some sort.

I only do code in JS a few times a year, so hence I'm asking for your time. Right now I'm working on little full-stack pice for my portfolio, and I'm really not a frontend developer :)

Here is the code:

class StreamDispatcher {
  constructor() {
    this._listeners = new Map();
  }

  register(obj, identifier, event, fn) {
    const objectTarget = this._digMap(this._listeners, [identifier, event], Map);
    const funcs = this._digMap(objectTarget, obj, Set);
    
    funcs.add(fn);
  }

  unregister(obj, identifier, event, fn) {
    const funcs = this._digMap(this._listeners, [identifier, event, obj]);
    funcs?.delete(fn);
  }

  unregisterAll(obj, identifier, event) {
    const objectTarget = this._digMap(this._listeners, [identifier, event]);
    objectTarget?.delete(obj);
  }

  dispatch(identifier, event) {
    const objectTarget = this._digMap(this._listeners, [identifier, event]);

    objectTarget?.forEach((funcs, obj) => {
      funcs.forEach(fn => fn.call(obj));
    });
  }

  _digMap(map, keys, initalizer) {
    keys = [...[keys]].flat();
    const key = keys.shift();

    let value = map.get(key);

    if(!value && initalizer) {
      value = new(initalizer);
      map.set(key, value);
    }

    if(!keys.length || !value) 
      return value;

    return this._digMap(value, keys, initalizer);
  }
}

Sample usage scenario:

const sd = new StreamDispatcher();

class App {
  constructor()
  {
    this.name = "name";
    sd.register(this, "List:1234", "print", this.printName);
  }

  printName() {
    console.log(this.name);
  }

  ondisable() {
    sd.unregisterAll(this, "List:1234", "print");
  }
}

const app1 = new App();

sd.dispatch("List:1234", "print"); //Output: name
app1.name = "Test";
sd.dispatch("List:1234", "print"); //Output: Test

app1.ondisable();
sd.dispatch("List:1234", "print"); //No output

Answer 1

Style nitpicks

• Be consistent with Allman vs Egyptian braces:

  constructor()
  {
    this.name = "name";
    sd.register(this, "List:1234", "print", this.printName);
  }
  

  should be:

  constructor() {
    this.name = "name";
    sd.register(this, "List:1234", "print", this.printName);
  }
  

• Use braces on all conditions and use whitespace around keywords:

  if(!keys.length || !value) 
    return value;
  

  goes to

  if (!keys.length || !value) {
    return value;
  }
  

_digMap feels overburdened

The function is confusingly named, which smells of a deeper design problem. I'm not sure what a dig is -- I assume you are "digging" through the multi-level Map...?

Functions that accept extra parameters to change the behavior tend to be problematic, especially when they mutate an object/context parameter in place. In _digMap, the function operates as a getter when the third parameter isn't provided and as a setter when it is. From the calling code's perspective, I have to squint (i.e. count parameters and look at _digMap's code) to figure out whether a given call is a get or a set operation.

The general strategy is to split overburdened functions into separate functions that don't accept a flag or optional parameter that selects the behavior. In the case of _digMap, you might separate the getter functionality from the setter functionality. Call the setter when you want to set, call the getter when you want to get.

You may worry that the above splitting operation introduces code repetition, but you can still separate the behaviors into two functions, then write a shared traversal subroutine to DRY the code out if necessary (I don't think this is necessary here). A little bit WET seems acceptable as long as it's a refactorable implementation detail and we present the caller with the cleanest, most intuitive interface possible -- that's the whole point of abstraction.

Avoid using recursion when a loop will do

Iterating an array with recursion in imperative languages like JS is generally poor design. In _digMap, every step over keys requires a linear shift() operation, duplication and redundant flattening of the array just to get one element, making the time complexity of _digMap unnecessarily quadratic. Writing a simple loop over the keys array is simpler to reason about (keys can be effectively read-only) and has less overhead.

Consider abstracting the nested map

StreamDispatcher is using _digMap to manipulate the _listeners map to such an extent that half of the code in the class is dedicated to data structure bookkeeping. The attention given to fussing with the map harms readability and distracts the StreamDispatcher class from focusing on its main goal: dispatching streams.

You want a structure that supports nested keying semantics like map.setNestedValue([foo, bar, baz], val) but stopped short of writing a class to abstract away all of the necessary busy work.

There's a builtin class in Python called defaultdict (dict is pretty much the same as a Map) that accepts a default constructor to fill in missing keys with, like dd = defaultdict(set). It's common to make these default dictionaries recursive, so you can go dd["foo"]["bar"]["baz"].add("quux") and have the keys and set created on the fly, then have "quux" added to the set. We can do something similar in JS but with traditional function call syntax. Proxies let you get closer to the Python syntax and there's an npm package that supports recursion, but I'll avoid proxies here.

By creating a new class NestedDefaultMap, we're taking value = new(initalizer); and _digMap's get/set pattern to their logical conclusion. This is a common refactor for doFooToMutateStatefulGarply(statefulGarply, /* more params */)-type calls. This C-style "OOP" can often be refactored to garply.Foo(params) where garply is an instance of class Garply.

After fully generalizing this data structure to its own class, your StreamDispatcher methods are all one-liners and the class is very clean. See below for implementation for NestedDefaultMap (I don't claim it to be a perfect design, just a proof-of-concept, and I may have overgeneralized it for this exact use case).

Too much sharing?

sd.register(this, "List:1234", "print", this.printName); feels like telling StreamDispatcher too much about this. We already have an identifier "list:1234" that's used to establish an event channel and this is only used for binding to fn by StreamDispatcher. Consider an interface that binds this in the client like:

sd.register("List:1234", "print", () => this.printName());

Here, the StreamDispatcher doesn't know what this is and there's no object context needed to invoke the handler.

An advantage of the redesign is that StreamDispatcher no longer requires this from a client, so events can be added and dispatched regardless of whether the client is an object or not. This redesign makes it easier for clients to bind parameters and simplifies the StreamDispatcher class.

Identifier clashes and overall purpose?

The fact that the client specifies an identifier feels a little odd to me because you can have clashes, but maybe that's intended. I'm not sure I follow the overall purpose of the class since there are no subscriptions. It feels like a fancy abstraction to invoke functions in another class without being able to access a return value, but I'm probably missing the point.

Avoid globals

In the tester code, App breaks encapsulation, relying on a global variable sd. If there's a dependency on this object, it should be passed into the constructor:

class App {
  constructor(streamDispatcher) {
    this.sd = streamDispatcher;
  }
  // ... use this.sd, not global sd ...
}

const sd = new StreamDispatcher();
const app = new App(sd);

Watch out for busy constructors

Minor point since I know App is just for testing, but it can be an antipattern to do too much in the constructor beyond assigning to this state. I'm a bit wary of sd.register in App's constructor, especially with a hardcoded value. In a real app, this would likely be a separate register() function with parameters.

Rewrite suggestion

class NestedDefaultMap {
  constructor(defaultFn) {
    this.defaultFn = defaultFn;
    this.root = new Map();
  }
  
  add(...keys) {
    let map = this.root;
    
    for (const key of keys.slice(0, -1)) {
      map.has(key) || map.set(key, new Map());
      map = map.get(key);
    }

    const key = keys[keys.length-1];
    map.has(key) || map.set(key, this.defaultFn());
    return {set: setterFn => map.set(key, setterFn(map.get(key)))};
  }
  
  get(...keys) {
    let map = this.root;

    for (const key of keys) {
      map = map?.get(key);
    }

    return map;
  }
}

class StreamDispatcher {
  constructor() {
    this._listeners = new NestedDefaultMap(() => new Set());
  }

  register(identifier, event, fn) {
    this._listeners.add(identifier, event).set(v => v.add(fn));
  }

  unregister(identifier, event, fn) {
    this._listeners.get(identifier, event)?.delete(fn);
  }

  unregisterAll(identifier, event) {
    this._listeners.get(identifier, event)?.clear();
  }

  dispatch(identifier, event) {
    this._listeners.get(identifier, event)?.forEach(fn => fn());
  }
}

// Sample usage scenario:

class App {
  constructor(name, streamDispatcher) {
    this.name = name;
    this.sd = streamDispatcher;
    this.sd.register("List:1234", "print", () => this.printName());
  }

  printName() {
    console.log(this.name);
  }

  ondisable() {
    this.sd.unregisterAll("List:1234", "print");
  }
}

const sd = new StreamDispatcher();
const app = new App("my name is foo", sd);
sd.dispatch("List:1234", "print"); // my name is foo
app.name = "my name is bar";
sd.dispatch("List:1234", "print"); // my name is bar
app.ondisable();
sd.dispatch("List:1234", "print"); // no output