10
\$\begingroup\$

I have been trying various formats of namespaces and modules patterns, however I have not come across a solution that I would use for all my projects.

I've been developing the following, that would allow me to separate my application through applications, modules, functions and global variables. Possibly I'm going to add the possibility to have public and private variables and methods.

/*--------------------------------------------------------------------------
 * Project name
 *
 * Created      : 01/01/2014
 * Modified     : 01/01/2014
 * Version      : 0.1
 * Developer    : Developer Name
 * 
---------------------------------------------------------------------------*/
/**
 * @class mainClassName
 * @constructor
 */
window.APP_NAME = window.APP_NAME || {};
window.APP_NAME.moduleName = window.APP_NAME.moduleName || {};
window.APP_NAME.deepExtend = window.APP_NAME.deepExtend || {};

window.APP_NAME.moduleName.globalVariableAvailableForEveryModuleFunctions = 0;

window.APP_NAME.moduleName.functionInsideModule1 = function (options) {

    "use strict";
    /*jslint nomen: true*/
    /*global $, jQuery*/


    // Check for arguments here.
    if (arguments.length !== 1) {
        throw new Error('[ClassName here] Please provide your options.');
    } // if

    // Increase global variable.    
    window.APP_NAME.moduleName.globalVariableAvailableForEveryModuleFunctions += 1;

    // Store module defaults.
    window.APP_NAME.moduleName.config = {
        name: 'John',
        age: 29
    };

    // Parse incoming object that stores config files.
    window.APP_NAME.deepExtend(window.APP_NAME.moduleName.config, options);

    // Rest of the application here.
    alert(window.APP_NAME.moduleName.config.name);

    // Call function in same module, to alert the age
    window.APP_NAME.moduleName.functionInsideModule2();

};


window.APP_NAME.moduleName.functionInsideModule2 = function () {

    "use strict";
    /*jslint nomen: true*/
    /*global $, jQuery*/

    // Store global in local var.
    var age = window.APP_NAME.moduleName.config.age;

    // Alert age stored in Module config.
    alert(age);

};




// Helper function to change config defaults.
window.APP_NAME.deepExtend = function (destination, source) {
    for (var property in source) {
        if (source[property] && source[property].constructor &&
            source[property].constructor === Object) {
            destination[property] = destination[property] || {};
            arguments.callee(destination[property], source[property]);
        } else {
            destination[property] = source[property];
        }
    }
    return destination;
};




// Let's run the application!
(function () {

    new window.APP_NAME.moduleName.functionInsideModule1({
        name: 'Mary'
    });

}());

I would like to receive some feedback on this kind of approach, and what you would change.

Variable and method names are long to provide a better understand of context.

\$\endgroup\$
5
\$\begingroup\$

Lets look at a couple of improvements:

new is creating an object that you aren't using. read edit for clarification

new window.APP_NAME.moduleName.functionInsideModule1({
    name: 'Mary'
});

The function functionInsideModule1 doesn't return an object and doesn't assign anything to this. So there is no need for the new. You're referencing the whole namespace window.APP_NAME.moduleName every time. Instead try putting it in a variable. Or maybe on the inside of the module refer to this instead.

(function () {

    var moduleName = window.APP_NAME.moduleName;

    moduleName.functionInsideModule1 = function (options) {
        /* snip ... */

        moduleName.globalVariableAvailableForEveryModuleFunctions += 1;
        // OR
        this.globalVariableAvailableForEveryModuleFunctions += 1;
    };

    moduleName.functionInsideModule1({
         name: 'Mary'
    });

}());

Edit:-

To create a new instance of moduleName you need to do this:

(function () {
    // private variables go here
    var globalPrivateCount = 0;

    var moduleName = window.APP_NAME.moduleName = window.APP_NAME.moduleName || function moduleName(initialval) {
        globalPrivateCount ++;

        this.globallyAccessible = 'unique for each instance';

        this.functionInsideModule1 = function (options) { ... };
        this.functionInsideModule2 = function (options) { ... };

        var instancePrivateVariable = initialVal;

        this.incr = function () { instancePrivateVariable++; };

        return this;
    };
    moduleName.nonUniqueGloballyAccessible = 0;
}());

and the usage is something like:

var moduleName = window.APP_NAME.moduleName;
var firstInstance = new moduleName(10);
// OR
var anotherInstance = new window.APP_NAME.moduleName(20);
\$\endgroup\$
  • \$\begingroup\$ Thanks for answering @JamesKhoury. Regarding 'new', imagine I do need various instances of the function. In this example I don't, but possibly I would need them. Isn't it the right way to do it? Only calling the function would retrieve me the same instance, causing the variables to be overwritten by the last function call values. \$\endgroup\$ – Ivo Pereira Mar 28 '14 at 10:28
  • \$\begingroup\$ @IvoPereira Depends if you want an instance of moduleName or functionInsideModule1 \$\endgroup\$ – James Khoury Mar 30 '14 at 23:10
  • \$\begingroup\$ Sorry I mispelled it. I was talking about instancing the moduleName itself. \$\endgroup\$ – Ivo Pereira Apr 1 '14 at 10:54
  • \$\begingroup\$ @IvoPereira then it is wrong. A new instance of moduleName needs you to do APP_NAME.moduleName = function () { ... } and var myinstance = new moduleName(). Would you like a specific example? \$\endgroup\$ – James Khoury Apr 1 '14 at 12:44
  • \$\begingroup\$ That is what I meant to, a new instance of moduleName. You said there was no need for new. But would I do it if I don't instantiate various moduleNames? \$\endgroup\$ – Ivo Pereira Apr 1 '14 at 14:37
4
\$\begingroup\$

I don't like this approach.

The biggest issue I have with this code is the fact that namespaces are attached to every lookup. Beyond being a bit long and awkward, it's hard to see the difference between two very similar looking symbol names when they start in exactly the same fashion. This is especially important when dealing with constructors, which are typically capitalized to distinguish them from ordinary functions. Confusing a function and a constructor is dangerous in JavaScript because it will typically generate lots of bugs, but few exceptions.

The other problem is access. Your code is heavily namespaced, but doesn't seem to provide much protection of access. Lots of variables in different namespaces seem to be visible to one another, and that's going to make life very difficult for you if you need to make this code testable. Cross-dependency should be handled by explicit dependency injection.

Rather than using long namespaces, I would instead break my modules into files that each execute in an instantly invoked function expression (these are sometimes called 'modules', which might make your nomenclature a little confusing). Within each, I would use non-namespaced symbol names, before finally mapping the interface to these symbols onto a global object.

This approach works well at varying scales, because the inner bodies of module files are still very simple, but there's still proper encapsulation of module-specific logic.

\$\endgroup\$
  • \$\begingroup\$ Hello @JimmyBreck-McKye, thanks for answering. Ok, the namespaces may be a little more organized, by saving the namespace in a unique var, that I would use for the preceding scopes. Thanks for the note about constructors and capitalized names. I have not worked over private variables in this example. Do you have any good example on dependency injection? I was thinking about breaking my modules into files, and this example I've posted it is a module. Do you have any other suggestions on how to make it a better "module"? :) \$\endgroup\$ – Ivo Pereira Mar 28 '14 at 10:15
  • 1
    \$\begingroup\$ instantly invoked function expression I think you'll find that immediately invoked function expression has become the more standardized expansion, though usually written as IIFE. \$\endgroup\$ – Jeremy J Starcher Apr 1 '14 at 20:11
  • \$\begingroup\$ @JeremyJStarcher I have already used an IIFE in to call new instances. \$\endgroup\$ – Ivo Pereira Apr 2 '14 at 8:57
4
\$\begingroup\$

From a once over:

  • In the below block, you can assume that if window.APP_NAME exists, that deepExtend is assigned, and if it did not exist, then obviously deepExtend is not assigned.

    So I would do this:

    window.APP_NAME = window.APP_NAME || {
    
        deepExtend : deepExtend = function (destination, source) {
            for (var property in source) {
                if (source[property] && source[property].constructor &&
                    source[property].constructor === Object) {
                    destination[property] = destination[property] || {};
                    arguments.callee(destination[property], source[property]);
                } else {
                    destination[property] = source[property];
                }
            }
            return destination;
        }
    }
    

    Wrapped inside an IIFE which would then have the "use strict";

  • On second thought, I don't like window.APP_NAME, just go for var APP_NAME, there is no good reason to attach straight to window, if you insist on doing this, then you should pass window to the IFFE I mentioned above.

  • deepExtend seems fine, though I would advise to see how jQuery does this, there are many corner cases in JavaScript

  • This is wrong :

    new window.APP_NAME.moduleName.functionInsideModule1({
        name: 'Mary'
    });
    
    • no reason to use window there
    • namespacing takes up horizontal space ( bad ) for no discernible benefit
    • if it is a constructor it should start with an uppercase F
      I would expect

      new moduleName.FunctionInsideModule1({
          name: 'Mary'
      });
      

      And even this seems ugly (your example function name is long..)

  • All in all, I am not a big fan of your approach, too hard to type, too hard to read, not a lot of benefits.

\$\endgroup\$
  • \$\begingroup\$ Thanks for answering @konijin. 1 - Seems good, but you would include all the functions inside that APP_NAME object? 2 - Imagine I have got APP_NAME inside an IIFE, other modules will not be able to access this one if I don't pass it to outside. 3 - I've looked exactly because of that. I didn't want to import all the library everytime I wanted to do some import. 4 - How would I get the namespace that is inside an IFFE? 5 - Do you have any good approach you would suggest me? \$\endgroup\$ – Ivo Pereira Mar 28 '14 at 10:25
1
\$\begingroup\$

I don't like your deepExtend for the following reasons:

  • You're not handling arrays which may contain nested objects no?
  • You don't handle the case where the destination property is an object but source property is a primitive. So for instance deepClone({x: {a: []}}, {x: "abc"}); will produce some fun results ;)
  • arguments.callee can not be used in strict mode (with good reason) and should be avoided. I would rather you recursively deep extend using an explicit function call.

Note, declaring your function as below only exposes the deepExtend variable name only inside of the function (because of a JavaScript oddity).

 window.APP_NAME.deepExtend = function deepExtend(destination, source) {
    /* ... */
    deepExtend(destination[property], source[property]);
 };
  • Your is object check is clever and prevents the function from extending classes like a RegExp. I'm pretty sure this line is redundant as any non null/undefined object will have a constructor AFAIK.

    source[property] && source[property].constructor && source[property].constructor === Object

Regardless, I would recommend you rewrite your is object check to use toString on the object. This is more reliable in case the user decides to give you some weird object like { x: {constructor: false}}

var toString = Object.prototype.toString;

/* .... */
toString.call(obj) === "[object Object]";

That said, I would recommend you investigate some of the more reputable deepClone or merge functions such as lodash, Mootools or this simple underscore implementation.

Personally, I think your function is probably fine after a few as below. Note I'm still not handling arrays which you may or not decide to include.

var toString = Object.prototype.toString;

// Helper function to change config defaults.
window.APP_NAME.deepExtend = function deepExtend(destination, source) {
    var destType, sourceType;
    for (var property in source) {
        destType = toString.call(destination[property]);
        sourceType = toString.call(source[property]);

        if (destType === sourceType && destType === "[object Object]") { //both must be objects otherwise choose source
            destination[property] = destination[property] || {};
            deepExtend(destination[property], source[property]);
        } else {
            destination[property] = source[property];
        }

    }
    return destination;
};
\$\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.