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.

Savage

Sorry, I didn't understand your question. Could you please explain it better? – Audible

Are you sure this isn't referencing a class or element? Not sure if I understand your question either. – Lightless

You just started at a new company and found something you're not sure about, and instead of asking your coworkers who know the code why it's that way you posted about it on SO? Seems like a bad way to start a new job. – Jennee

Is this the actual code? or is it enclosed within something else? this could be anything, depending on the surroundings (but as I see it, this does seem refer to the global object) – Corse

There something else that looks wrong: the var statement should be on the first line, not the second. – Shoop

@FabrícioMatté yes, this refers to the window. See by your own – Audible

@bfavaretto. Yep I wrote that couple of min ago. – Parse

@Parse Sorry, hadn't seen your answer yet when I added that comment. – Shoop

Are you certain the code doesn't end with }.call({}));? – Counterpoise

Everything IS being added to window, I checked with Firebug. And no, it doesn't end with }.call({}));. – Savage

are you sure they are adding all the methods to this/that? That's amazing. Please post the post-mortem :) The only reason you would do this is to support non-browser environments, adding methods to the global object which might not be window. – Backwards

If the salary doesn't worth it, I suggest you find a new job... Bad programming is like a disease. – Parse

So... now that you know they suck, what are you going to do...? :) – Parse

Looking at the repo history it looks like one person is responsible for all the code that has this error, so I don't think it's a systemic problem at the company. I guess I'll have a word with the person tomorrow. – Savage

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!

Parse

@MarkLinus. They want to pollute the global object?! I believe they don't. Anyway if you want to do something wrong and you did something wrong- it's still wrong... :) – Parse

Declaring the globals is good practice, but makes no difference here. – Burlesque

@RobG. But this code doesn't have to be in the global object... it can be in an inner scope as well. – Parse

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.

Burlesque