1
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ Nothing stands out to me. Have you altered anything since the last good grade? Is there a configuration file? "Exceeds 47 lines of code." Given your small functions that seems terribly naive. No single function is "too long." Play w/ some simple ideas, but what's the point given obtuse rules? I would not break apart the "class" for the sake of an arbitrary number - that's a recipe for making it worse. If you're paying for this tool call them and ask. \$\endgroup\$ – radarbob Mar 11 '18 at 8:46
  • \$\begingroup\$ I didn't alter anything, it was their scoring system that changed. There is no config file either. I do wonder though, why the negative points ? Isn't this a specific / good question for code review? \$\endgroup\$ – Flame_Phoenix Mar 11 '18 at 9:19
  • \$\begingroup\$ @Flame_Phoenix "Isn't this a specific / good question for code review?" Not really. It seems to be opinion based. You are looking for an answer to confirm if that tool is right or wrong, rather than just providing your code to be inspected for improvements. \$\endgroup\$ – πάντα ῥεῖ Mar 11 '18 at 9:25
  • \$\begingroup\$ Perhaps I am not explaining myself well enough. My idea is since it is an automated process, it should be right, but I am not capable enough to see where improvements can be made. This is why i posted the code here for a review. Should I rephrase the question? Or is it simply too subjective in nature ? \$\endgroup\$ – Flame_Phoenix Mar 11 '18 at 9:47
  • 3
    \$\begingroup\$ I think that this is a fine question for Code Review, with the title modified to conform to the How to Ask guidelines. \$\endgroup\$ – 200_success Mar 11 '18 at 11:30
0
\$\begingroup\$

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
};
\$\endgroup\$
  • \$\begingroup\$ No, because that way the map will be shared with all watchers, and the idea is for each watcher to have its own map. You are pretty close to another solution though :P \$\endgroup\$ – Flame_Phoenix Mar 11 '18 at 20:47
  • \$\begingroup\$ @Flame_Phoenix Don't you only export a single watcher? \$\endgroup\$ – Solomon Ucko Mar 11 '18 at 20:49
  • \$\begingroup\$ A typo! Thanks for noticing! The idea actually export a factory of watchers, so you can have multiple ! \$\endgroup\$ – Flame_Phoenix Mar 11 '18 at 20:52
  • \$\begingroup\$ @Flame_Phoenix Great! \$\endgroup\$ – Solomon Ucko Mar 11 '18 at 21:11
0
\$\begingroup\$

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);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.