2
\$\begingroup\$

By outer-package, I mean outside of this package - this can be the DOM or other packages. This package has a basic event system and a basic registry. It also has a debugger which allows you to fiddle with the DOM by removing and adding entire sets of tags. Later it might have things which allow it to Debug / Configure the server via ajax.

Please review all aspects of this code.

/***************************************************************************************************
    **COMS
    **  provides registry, event system, debugger ...
    **  reduces dependencies
    ***************************************************************************************************/

    (function () {
        "use strict";
        var $A,

            // public
            $P = {};

        // require utility
        (function manageGlobal() {
            if (window.$A && window.$A.pack && window.$A.pack.utility) {
                $A = window.$A;
                $A.pack.comms = true;
            } else {
                throw "comms requires utility module";
            }
        }());
        $P.Debug = (function () {
            var publik = {},
                hold = {};
            publik.addTags = function (tag) {
                if (hold[tag]) {
                    $A.someIndex(hold[tag], function (val) {
                        if (tag === 'script' || tag === 'style') {
                            document.head.appendChild(val);
                        } else {
                            document.body.appendChild(val);
                        }
                    });
                }
            };
            publik.removeTags = function (tag) {
                var styles = document.getElementsByTagName(tag),
                    i;
                hold[tag] = [];
                for (i = styles.length; i; i--) {
                    hold[tag][i] = styles[i];
                    $A.removeElement((styles[i]));
                }
            };
            publik.removeStorage = function () {
                localStorage.clear();
                sessionStorage.clear();
            };
            return publik;
        }());

        // a basic registry pattern with get/set and getMany/setMany
        $P.Reg = (function () {
            var publik = {},
                register = {};
            publik.get = function (key) {
                return register[key];
            };
            publik.set = function (key, value) {
                register[key] = value;
            };
            publik.setMany = function (o) {
                $A.someKey(o, function (val, key) {
                    register[key] = val;
                });
            };
            publik.getMany = function () {
                return register;
            };
            return publik;
        }());

        // a basic event system using an internal bus
        $P.Event = (function () {
            var publik = {},
                events = {};
            publik.add = function (name, callback) {
                if (!events[name]) {
                    events[name] = [];
                }
                events[name].push(callback);
            };
            publik.remove = function (name, callback) {
                if (name && callback) {
                    delete events[name][callback];
                } else if (name) {
                    delete events[name];
                }
            };
            publik.trigger = function (name) {
                if (events[name]) {
                    $A.someIndex(events[name], function (val) {
                        val();
                    });
                }
            };
            return publik;
        }());
        window.$A = $A.extendSafe($A, $P);
    }());
\$\endgroup\$
3
\$\begingroup\$

The below code would be cleaner with object literal notation :

    // a basic registry pattern with get/set and getMany/setMany
    $P.Reg = (function () {
        var publik = {},
            register = {};
        publik.get = function (key) {
            return register[key];
        };
        publik.set = function (key, value) {
            register[key] = value;
        };
        publik.setMany = function (o) {
            $A.someKey(o, function (val, key) {
                register[key] = val;
            });
        };
        publik.getMany = function () {
            return register;
        };
        return publik;
    }());

would be

    // a basic registry pattern with get/set and getMany/setMany
    $P.Reg = (function () {
        var register = {};
        return {
            get : function (key) {
                return register[key];
            },
            set : function (key, value) {
                register[key] = value;
            },
            setMany : function (o) {
                $A.someKey(o, function (val, key) {
                    register[key] = val;
                });
            },
            getMany : function () {
                return register;
            }
        };
    }());

I also note that

  • getMany is wrongly named, getAll perhaps ?
  • not sure what someKey does, is it any different from for( var key in o) ?
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ some() applies a test before executing the callback, you seem to have cloned forEach()? \$\endgroup\$ – konijn Jan 6 '14 at 14:58
1
\$\begingroup\$

I agree object notation would look cleaner in this case.

I don't see why you need Reg. It's a very thin wrapper over an object.
There's no need to implement “registry pattern” in JS because every JS object already is a “registry”.

I'm not sure why you need a Debug helper that loads and unloads external scripts or styles. In fact, I don't understand its purpose at all: addTags accepts a tag parameter, which presumably is a string (you're using it as a key), but supposedly the first call will do nothing, since hold[tag] will be empty. Do I need to call removeTags first? What is a tag anyway? The code seems simple, but I fail to understand what it does and how you use it. Maybe you need to ditch simple names in favor of more descriptive ones?

As for putting these utilities under a common umbrella of a “package for outer-package communications”, I stay unconvinced. These utilities have little to do with each other, and the way you describe it, grouping them doesn't sound convincing.

By outer-package, I mean outside of this package - this can be the DOM or other packages. This package has a basic event system and a basic registry. It also has a debugger which allows you to fiddle with the DOM by removing and adding entire sets of tags. Later it might have things which allow it to Debug / Configure the server via ajax.

It may seem like each module needs a registry, or some debug tools, or event system, but you can already achieve this without grouping them:

  • put Pubsub separately and make it a mixin like Backbone.Events;

  • debug tools can also be kept separately;

  • any JS object is a “registry” so no real need for that.

Otherwise, to me, this package looks like a “God module”.

Not that there's something wrong with it, if you meant it to be all-the-utilities-I-will-ever-need kind of module. I wasn't sure from your question if you meant it.

The nice thing about separating even small utilities is that you can

  • open source them because they are decoupled from your code, and others can hack on them;

  • replace them one by one with different implementations;

  • they are immune to “specifics creep”, i.e. the situation when over time your once-generic utilities become too tightly coupled to one specific project, and grow too difficult to disentangle so you have to rewrite them once again for your next project.

| improve this answer | |
\$\endgroup\$

Your Answer

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