this keyword in module pattern?

Question

I've just started working at a new company and noticed something that looks completely wrong to me in a lot of their JS. I'm a bit hesitant to bring it up without confirming this is wrong since I'm pretty junior, I'm not a JS expert and it's only my second day and I don't want to look stupid.

So, normally I'd expect the module pattern to look something like:

MODULENAME = MODULENAME || {};

MODULENAME.SUBMODULENAME = (function() {
    var bla = {};

    bla.somefunction = function() {
        //do stuff
    };

    //add more stuff to bla
    return bla;
}());

What they have all over their code is:

MODULENAME = MODULENAME || {};

MODULENAME.SUBMODULENAME = (function() {
    var that = this;

    that.somefunction = function() {
        //do stuff
    };

    //add more stuff to that
    return that;
}());

Now of course because the function isn't being called as a constructor with the new keyword or as a method, this is bound to window and they're defining that as this. So they're basically dumping everything in the global object and all their sub-module names are in fact aliases for window. Is there any reason anyone would want to do this? Or is this really as wrong as it seems to me?

Edit:

I made a mistake in putting var before the submodule definition, originally I wrote something slightly different and forgot to delete the var. I've tried to make the example a bit clearer too, hopefully it's more obvious what I mean now.

Edit 2:

Also I've looked at the scripts executing in Firebug and they are definitely adding everything to window, that object is a total mess.

Answer 1

Yes it looks wrong.

MODULENAME = MODULENAME || {}; // missing var

var MODULENAME.SUBMODULENAME = (function() { // probably the missing var from above...
    var that = this;
    //add some stuff to that
    return that; // that is the WINDOW- wrong.
}());

DEMO for the damage it can do:

var x = function() {
    alert('out');
}
var MODULENAME = MODULENAME || {};

MODULENAME.SUBMODULENAME = (function() {
    var that = this;
    that.x = function() {
        alert('DAMAGE');
    }
}());

x();​ // alert DAMAGE and not "out" - messed up with the global object!

Answer 2

The module pattern is being used incorrectly, and one reason why function expressions should not be used where their use provides nothing over a function declaration. If the intention is to create global functions (which I doubt it is), then they should use:

function somefuncion() {
  ...
}

If their intention is add properties (in this case methods) to an object, which is more likely the case, then:

MODULENAME.SUBMODULENAME.somemethod = function() { /* do stuff */ };

If there is a need to conditionally create methods, e.g. based on feature detection, then the following may suit:

(function(global, undefined) {

  // In here global is the global object
  global.MODULENAME = global.MODULENAME || {};
  global.MODULENAME.SUBMODULENAME = global.MODULENAME.SUBMODULENAME || {};

  // and undefined is undefined, belt and braces approach
  undefined = void 0;

  // Direct assignment
  function somemethod() {
      //do stuff      
  };

  // Assign directly to the "namespace" object
  MODULENAME.SUBMODULENAME.somemethod = somemethod;

  // Conditional assignment
  if ( sometest ) {
    MODULENAME.SUBMODULENAME.anothermethod = function(){...};

  // Try another way...
  } else if (someOtherTest) {
    MODULENAME.SUBMODULENAME.anothermethod = function(){...};

  // Default
  } else {
    MODULENAME.SUBMODULENAME.anothermethod = function(){...};
  }

  // Clean up 
  global = null;

}(this)); 

One issue with the above is that every function declared inside the outer function has a closure back to the function object and its environment, so it's a bit wasteful of resources. It's much more efficient to keep it simple and only use the module pattern where it's really needed and just use plain function declarations and assignments where it isn't. Not as funky but more practical.