Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Simplified working demo code here.

Relevant JS code:

;(function(exports) {

    //--------------------------------------------------------------------------
    //
    // Private properties:
    //
    //--------------------------------------------------------------------------

    var _msg = '',
    _ol = document.getElementById('debug');

    //--------------------------------------------------------------------------
    //
    // Public methods:
    //
    //--------------------------------------------------------------------------

    exports.init = function(arg) {

        _msg = arg;

        // Add a listener to the `window.resize` event, pass `exports`/`self` as the scope:
        _addEvent(window, 'resize', _listenForChange, exports);

    };

    //--------------------------------------------------------------------------
    //
    // Private methods:
    //
    //--------------------------------------------------------------------------

    function _addEvent(elem, type, handle, context) {

        // Attache event:
        if (elem) {

            if (elem.addEventListener) {

                // If the browser supports event listeners, use them:
                elem.addEventListener(type, function() { handle.call(context); }, false);

            } else if (elem.attachEvent) {

                // IE & Opera:
                elem.attachEvent('on' + type, function() { handle.call(context); });

            } else {

                // Otherwise, replace the current thing bound to `on[whatever]`!
                elem['on' + type] = function() { handle.call(context); };

            }

        }

    }

    function _listenForChange() {

        var li = document.createElement('li');

        li.innerHTML = String(_msg);

        _ol.appendChild(li);

        if (typeof console !== 'undefined') console.log(_msg);

    }

    return exports; // Expose the API.

}(window.FOO = window.FOO || {}));

//--------------------------------------------------------------------

window.onload = function() {

    FOO.init('world');

    FOO.init('universe');

};

Problem with existing code:

When re-sizing the window, I only see "universe".

Goal:

When browser window re-sizes, I want to world "world" and "universe" output as a list item (and to the console).

I want to have the ability to instantiate FOO multiple times and have it work independently from any other running instances.

My questions:

Is there a pattern (or patterns), similar to the one(s) that I'm currently using, that will allow me to run multiple instances of FOO without having the last FOO called trump all previously called FOOs?

The code I posted is obviously not real world; my end goal is to allow/modify a plugin I wrote to be used without the end user having to worry if FOO is already being used by another script.


EDIT #1

Here's a JSBin version of my example code that incorporates Abuzittin's (and Joseph's) suggestions. Many thanks for the help folks!

Unfortunately, I was not able to get it to work out of the box. Though, based on those suggestions, I came up with this:

;(function(exports) {

    var _ol = document.getElementById('debug');

    exports.init = function(arg) {

        this._msg = arg;

        // Add a listener to the `window.resize` event, pass `exports`/`self` as the scope:
        this.addEvent(window, 'resize', exports.listenForChange, exports);

        exports.listenForChange(); // Fire off first time.

    };

    exports.listenForChange = function() {

        var msg = this._msg;

        var li = document.createElement('li');

        li.innerHTML = String('Hello ' + msg);

        _ol.appendChild(li);

        if (typeof console !== 'undefined') {

            console.log(msg, '...');

        }

    };

    exports.addEvent = function(elem, type, handle, context) {

        // Attache event:
        if (elem) {

            if (elem.addEventListener) {

                // If the browser supports event listeners, use them:
                elem.addEventListener(type, function() { handle.call(context); }, false);

            } else if (elem.attachEvent) {

                // IE & Opera:
                elem.attachEvent('on' + type, function() { handle.call(context); });

            } else {

                // Otherwise, replace the current thing bound to `on[whatever]`!
                elem['on' + type] = function() { handle.call(context); };

            }

        }

    };

    return exports; // Expose the API.

}(window.FOO = window.FOO || {}));

//--------------------------------------------------------------------

window.onload = function() {

    FOO.init('world');

    FOO.init('universe');

};

... which works (JSBin example), but only on init (like Abuzittin was saying).

Updated question:

Is it it even possible to have multiple copies of window.FOO? Is there some way to allow the end user to define what FOO is? Or, could I setup my pattern to allow for the user to say FOO.CUSTOMINSTANCENAME.init()?

NOTE:

I had originally asked my question on StackOverflow; I soon realized that I needed to move my question here, to CodeReview.

On Stack, user fab had recommended:

function initFoo(fooName) {
    (function(foo) {

        foo.init = function() {};

        // other public/private methods here.

        return foo;

    }(window[fooName] = window[fooName] || {}));
}

initFoo('FOO');
initFoo('BILLY');

In your opinions, based on the code I have provided, would that be my best option? Are there alternatives?

Also, Oerd recommended I read:

Addy Osmani's Essential JS Namespacing Patterns

... Which is an awesome article! I'm just having troubles trying to figure out if any of those namespace patterns mentioned in the article would meet the needs of my code.

share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

There is only one copy of window.FOO and window.FOO.init, despite its name does not create new objects. window.FOO.init just overwrites _msg that (function(exports) {... closed over each time it is called. _listenForChange() holds a reference to the closed over _msg. If you want it to keep a copy of the value at the time init is called then you can do like so:

_addEvent(window, 'resize', listenForChange2(), exports);

var listenForChange2 = function() {
    var msg = _msg; // keep own copy of the _msg
    return function() {
        var li = document.createElement('li');
        li.innerHTML = String("second version " + msg);
        _ol.appendChild(li);
        if (typeof console !== 'undefined') console.log(msg);
    }
}

Review of the code posted in the EDIT #1

First of all you are still confused about closures. It is important that you understand them because they are how you associate some state with a function. But explaining closures is not within the scope of Code Review. And you probably won't get any better answers than these, the accepted answer in this SO question and this article linked from there. You can fix your bug if you understand these.

Also the answer above fixes the bug if you KEEP the parens here:

_addEvent(window, 'resize', listenForChange2(), exports);

I the JSBIN you posted just add the parens and It works. So I will not repeat how can you fix the answer in the new code.

Onto the review. I will go over the code top down:

    exports.init = function(arg) {

By convention init method of an object is used for object initialization. It is not what it does here. Better name

       this._msg = arg;

You need not keep a copy of _msg since it will not be used elsewhere. You prepend the name with an underscore as if to indicate it is a private member (which practice I do not recommend BTW), but then expose it as public by assigning to this.. You can see, in the Firebug console for example, that Foo._msg is defined.

    exports.listenForChange = function() {

You need not expose this either.

    var listenForChange = function() {

would be just as good.

        var msg = this._msg;

If you do not create a new function, copying the value does not do anything.

    exports.addEvent = function(elem, type, handle, context) {

You need not expose this either, as it will not be called like Foo.addEvent.

Also adding another layer of indirection solving any problem, does not apply here. Whenever there is something you do not understand, namely a bug, adding another layer exacerbates the problem. What does this refer to when the listenForChange function is run?

Even if you fix your bug this may introduce another one:

                elem['on' + type] = function() { handle.call(context); };

You are overwriting the onclick. Only the last handler assigned to the onResize will fire.

This is unnecessary:

    return exports; // Expose the API.

No one is using the returned value. Moreover the comment is misleading as everything you assingn to exports. will be exposed, whether you return it or not.

share|improve this answer
    
On the _addEvent(), listenForChange2() should be just listenForChange2. It should not be a call, just a passing of listenForChange2 to _addEvent(). –  Joseph the Dreamer Feb 1 '13 at 13:51
    
Thank you abuzittin-gillifirca and @joseph-the-dreamer, I really appreciate the pro help! I've updated my question (see "EDIT #1") to show my progress based on your feedback. If you have the spare time, I'd love to know what your thoughts are on my updated code/question. –  mhulse Feb 1 '13 at 19:42
    
@abuzittin-gillifirca just curious if you have any feedback on the edit to my answer? Using your suggestion, I was only able to get "world" to output on init and everything else was "universe". Is there a pattern, or a way, for me to use var foo1 = New FOO(), foo2 = New FOO(); foo1.init('world'); foo2.init('universe');? I've nevered used New in a plugin before... Seems like that would do what I want, but would you recommended it based on what you've seen of my code? Thanks! –  mhulse Feb 11 '13 at 20:44
    
@MickyHulse First of all in this jsbin.com/efeyuw/2 version you NEED the parentheses after listenForChange2, because the function you need is the result of calling listenForChange2 not listenForChange2 itself. Note listenForChange2 creates a new function each time it is called that is how it is possible to have different events firing. I will try review the newly posted code as soon as job permits. code cannot be posted in comments, so i will probably append it to the previous answer.. –  abuzittin gillifirca Feb 12 '13 at 7:09
    
@abuzittingillifirca WOW, thank you!!!! Your latest update really helped to clarify things for me. Thanks so much for taking the time to help me out, I really appreciate it! I owe you one. Have an awesome day. :) –  mhulse Feb 12 '13 at 20:53
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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