this in js.md

Is this worth it?

I've been editing this in response to comments below, but I will preserve the full revision history.

Referencing the function execution context via this is fairly common in JavaScript. I suspect some people like it because they come from a language like Java, and it’s familiar. However, I would argue that it in some cases introduces brittleness into one's code without bringing much benefit.

For example, let’s consider a contrived logger module:

function CountingLogger() {
  if (!(this instanceof CountingLogger)) return new CountingLogger;
  this.count = 0;
}

CountingLogger.prototype.log = function (msg) {
  console.log(this.count, msg);
  this.count++;
}

It would be used as follows:

var logger = new CountingLogger();
logger.log('foo'); // => 1 foo
logger.log('bar'); // => 2 bar

That works fine. However, the problem is that by using this, we’ve made an assumption about the way the method will be used. If the caller tries to use the method differently, they’re gonna have a bad time:

var logger = new CountingLogger();
['foo', 'bar'].forEach(logger.log);
// => undefined foo
// => undefined bar

Why is it acceptable to fail in this case? I would argue that it is not. We happen to not even be throwing an error, but we're violating the principle of least astonishment and requiring the user to understand implementation details of our module.

In some situations, this could cause an error:

this.someProperty.doSomething();

JavaScript doesn’t even provide a clear error message, like Error: This function requires a context that has property 'someProperty', but was called without one.. Instead, the caller gets some random error at the first place that this is required, and has to look into our code to figure out what’s going on. (Worse yet, imagine if someProperty also existed on window, which is what this refers to if not explicitly specified - then we could get in to some truly confusing behavior.) This is not a good user experience for consumers of our module.

We can fix the above example by using bind() (or an equivalent helper method for older browsers):

var logger = new CountingLogger();
['foo', 'bar'].forEach(logger.log.bind(logger)); 
// => 1 foo
// => 2 bar

This is fine, but why make our users go through this in the first place? We can easily refactor our module to use closures instead:

function createCountingLogger() {

  var count = 0;

  function log(msg) {
    console.log(count, msg);
    count++;
  }

  return {
    log: log
  };
}

Now the user is not affected by our implementation details:

var logger = createCountingLogger();
['foo', 'bar'].forEach(logger.log);
// => 1 foo
// => 2 bar

But what about chaining?

A common use of this is to provide chaining, which can create nice APIs. Here is an example of the logger module that supports chaining:

function CountingLogger() {

  this.count = 0;

  this.log = function (msg) {
    console.log(this.count, msg);
    this.count++;
    return this;
  }

}

Chaining is fun:

var countingLogger = new CountingLogger();
countLogger
  .log('foo')  // => 1 foo
  .log('bar'); // => 2 bar 

We can easily use closures here as well to avoid having to use this:

function createCountingLogger() {

  var count = 0,
      countingLogger = {};

  function log(msg) {
    console.log(count, msg);
    count++;
    return countingLogger;
  }

  countingLogger.log = log;

 return countingLogger;
}

Now we get the benefits of a chained API with the robustness of this-free code.

Privacy

this is generally treated as private, internal state, but it can be overridden with .call, .apply, or .bind. If you intend for this to happen, then take whatever information this is giving you as an (optional) argument. If you do not intend for this to happen, don’t rely on this.

What about inheritance?

There are many good arguments for preferring composition over inheritance which I will not repeat here. That said, sometimes inheritance is truly what you need, in which case using this may be the right path.

@getify provides an intriguing argument for delegation-oriented instead of class-oriented organization that ditches this and may end up being less confusing.

Do we really need this?

There’s additional complexity generated to handle this, such as q.ninvoke, q.nbind, and any time someone feels the need to write var self = this. Is it really worth it?

Do you think this is ridiculous? Am I missing something? Let me know!

You're right that my tone was too aggressive. Thanks for pointing that out; I've scaled it back accordingly.

That's a neat trick with if (!(this instanceof CountingLogger)). I hadn't seen that one before. I like that as a way to make it less error-prone.

To be clear, I'm not advocating for a giant closure soup where everything you could ever need is in scope all the time. That's no good. I think the json source you linked to is difficult to read because of undescriptive variable names and the many LOC in that one function (which leads to closure soup), but neither of those issues are inherent to closure-oriented code.

the state that you actually need in each function is much more explicit

Is it really more explicit? If we have a method:

function blastOff(arg1, arg2, arg) {
  // ...
}

We can look at the declaration and easily see what its inputs are, but to know if this is also an input, we have to go digging through the source itself. I can see how this would be a difference of taste, and I wouldn't think this conversation would be worth having if the difference of taste didn't have real implications for end-users.

The main issue that I have this this is that it causes confusing errors when the user calls methods in a way that seems like it should be fine. This problem still occurs with the following:

function CountingLogger() {
  if (!(this instanceof CountingLogger)) return new CountingLogger;
  this.count = 0;
}

CountingLogger.prototype.log = function (msg) {
  console.log(this.count, msg);
}

var logger = new CountingLogger();
['foo', 'bar'].forEach(logger.log);
// => undefined 'foo'
// => undefined 'bar'

I agree that dropping an indentation level is nice, but I don't think it's worth the cost.

Do you think that ['foo', 'bar'].forEach(logger.log); is not a reasonable thing to expect to work? That could explain our difference of opinion.

it's really much more of an unimportant stylistic thing

I agree that it's a mistake to categorically reject ever using inheritance. I also agree that the difference between the code style of the prototypical or closure-based inheritance is a matter of personal taste. But I don't agree that's it's an "unimportant stylistic thing" when the choice can break functionality for the end user.