What's wrong (or right) with this JS Object Pattern?

Question

Here's an example of the pattern I'm using in my javascript objects these days (this example relies on jQuery). http://pastie.org/private/ryn0m1gnjsxdos9onsyxg

It works for me reasonably well, but I'm guessing there's something wrong, or at least sub-optimal about it, I'm just curious to get people's opinions.

Here's a smaller, inline example of it:

sample = function(attach) {
    // set internal reference to self
    var self = this;

    // public variable(s)
    self.iAmPublic = true;

    // private variable(s)
    var debug = false;
    var host = attach;
    var pane = {
        element: false,
        display: false
    }

    // public function(s)
    self.show = function() {
        if (!pane.display) {
            position();
            $(pane.element).show('fast');
            pane.display = true;
        }
    }

    self.hide = function() {
        if (pane.display) {
            $(pane.element).hide('fast');
            pane.display = false;
        }
    }

    // private function(s)
    function init () {
        // do whatever stuff is needed on instantiation of this object
        // like perhaps positioning a hidden div
        pane.element = document.createElement('div');
        return self;
    }

    function position() {
        var h = {
            'h': $(host).outerHeight(),
            'w': $(host).outerWidth(),
            'pos': $(host).offset()
        };
        var p = {
            'w': $(pane.element).outerWidth()
        };
        $(pane.element).css({
            top: h.pos.top + (h.h-1),
            left: h.pos.left + ((h.w - p.w) / 2)
        });
    }

    function log () {
        if (debug) { console.log(arguments); }
    }

    // on-instantiation let's set ourselves up
    return init();
}

I'm really curious to get people's thoughts on this.

Answer 1

sample = function(attach) {
    // set internal reference to self
    var self = this;

Why are don't you use this directly? Write Javascript, the one maintaining your code will thank you.

    // public variable(s)
    self.iAmPublic = true;

    // private variable(s)
    var debug = false;
    var host = attach;
    var pane = {
        element: false,
        display: false
    }

    // public function(s)
    self.show = function() {
        if (!pane.display) {
            position();
            $(pane.element).show('fast');
            pane.display = true;
        }
    }

    self.hide = function() {
        if (pane.display) {
            $(pane.element).hide('fast');
            pane.display = false;
        }
    }

I know that this is good OOP design, but really, you are adding yet another layer of indirection to plain Javascript. You'll have some things that you'll hide with $(e).hide('fast') and others that you'll hide using e.hide(). That inconsistency might make your code a bit more confusing than necessary.

    // private function(s)
    function init () {
        // do whatever stuff is needed on instantiation of this object
        // like perhaps positioning a hidden div
        pane.element = document.createElement('div');
        return self;
    }

Preppend a _ or __ before private methods and variables, as Javascript doesn't have private attributes, so you have to rely on convention, or use closures.

    function position() {
        var h = {
            'h': $(host).outerHeight(),
            'w': $(host).outerWidth(),
            'pos': $(host).offset()
        };
        var p = {
            'w': $(pane.element).outerWidth()
        };
        $(pane.element).css({
            top: h.pos.top + (h.h-1),
            left: h.pos.left + ((h.w - p.w) / 2)
        });
    }

    function log () {
        if (debug) { console.log(arguments); }
    }

This piece of code is a bit suboptimal, as the function log() will be called from outside the class. I would definitely wouldn't leave it on production code.

    // on-instantiation let's set ourselves up
    return init();
}