JavaScript library to notify watchers of changes

Question

Background

Last year, while I was still entering the beginning of Immutability and Pure functions and whatnot I created a library for the community called obj-watcher.

This library is extremely simple - it allows you to register an object, and to get notified when changed happen to this object via a given API.

Problem

Everything was fine at first. I got the maximum score for everything and then one day ... CodeClimate changed the way they evaluate code and they rated my project as a C.

This is not good. According to them, my project is hard to maintain because it has a high cognitive load. I find that hard to believe, but I am totally biased.

What do I want?

The full code is commented via jsdoc and I know I could probably improve some functions. But I what I would really like is some feedback on my code. I would like to know if it is really that hard for someone else to understand ( PS, removed the comments ):

const isFunction = require("lodash.isfunction");

const errors = require("./errors.js");
const callbackNotAFunction = errors.callbackNotAFunction;
const objectAlreadyWatched = errors.objectAlreadyWatched;
const objectNotWatched = errors.objectNotWatched;

const watcherFactory = () => {


    const watchMap = new Map();

    const watch = (objId, obj) => {
        if (isObjWatched(objId))
            throw objectAlreadyWatched(objId);

        watchMap.set(objId, {
            obj: obj,

            onChange: () => {}
        });
    };


    const unwatch = objId => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);
        watchMap.delete(objId);
    };


    const get = objId => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);
        return Object.assign({}, watchMap.get(objId).obj);
    };


    const set = (objId, newObj) => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);

        const entry = watchMap.get(objId);
        const oldObj = Object.assign({}, entry.obj);
        entry.obj = Object.assign({}, newObj);
        entry.onChange(oldObj, entry.obj);
    };


    const onChange = (objId, callback) => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);

        if (!isFunction(callback))
            throw callbackNotAFunction(objId);

        const entry = watchMap.get(objId);
        entry.onChange = callback;
    };


    const reset = () => {
        watchMap.clear();
    };

    const isObjWatched = objName => watchMap.has(objName);

    return Object.freeze({
        watch,
        unwatch,
        onChange,
        get,
        set,
        reset
    });
};

module.exports = watcherFactory();

CodeClimate states this code is hard due to 2 reasons:

• Function watcherFactory has 47 lines of code (exceeds 25 allowed). Consider refactoring.
• Function watcherFactory has a Cognitive Complexity of 12 (exceeds 5 allowed). Consider refactoring.

Here I am using the well established object factory pattern from Douglas Crockford where I have a function that creates and returns an object with methods.

This function has everything inside and is a standard way of defining public and private variables via means of closures and encapsulation.

However, after reading CodeClimate's article on cognitive complexity ( which I also recommend ):

https://docs.codeclimate.com/docs/cognitive-complexity

I think their system has an issue with factory functions because it seems them as normal function that is far too big. In a way, their system does kinda have a point... I wonder if there is a way I can refactor this to make it more obvious and win both battles.

What do you think?

Answer 1

Note: this ended up catching a typo instead of fixing what I said below: it was not meant to be executed immediately.

Why do you need something that's effectively an IIFE? Can't you just use:

const watchMap = new Map();

const watch = (objId, obj) => {
    if (isObjWatched(objId))
        throw objectAlreadyWatched(objId);

    watchMap.set(objId, {
        obj: obj,

        onChange: () => {}
    });
};

// ...

module.exports = {
    watch,
    unwatch,
    onChange,
    get,
    set,
    reset
};

Answer 2

By the looks of it I'd agree that their system just doesn't agree with factory functions. On one hand I'm inclined to agree with them that using the minimum of nested closures is better, but also keeping code separated can help you understand how it fits together better.

I can think of 2 options that would likely make CodeClimate score your project better, but not being a user I can't guarantee that these will help.

The first is probably the simplest; instead of using a factory define a class for your watcher, possibly inheriting from Map.

The second is to define your functions external to the factory and then use a binding function to wrap a map to each function.

const bindMethod = (map, fn) => (...args) => fn(map, ...args);

const isObjectWatched = (watchMap, objId) => watchMap.has(objId);

const watch = (watchMap, objId, obj) => {
    if (isObjWatched(watchMap, objId))
        throw objectAlreadyWatched(objId);

    watchMap.set(objId, {
        obj: obj,

        onChange: () => {}
    });
};

const watcherFactory = () => {

    const watchMap = new Map();

    return Object.freeze({
        watch: bindMethod(watchMap, watch)
    });
};

module.exports = watcherFactory;

This keeps the same functionality while keeping all the functions to a short length. Also other than the magic "bindMethod" function it's all fairly self explanatory.

Also here's an alternative version of the bind function, which is a bit more readable.

function bindMethod (map, fn) {
    return function (...parameters) {
        return fn(map, ...parameters);
    }
}