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!

this is fine and in many instances preferable to closures because prototypical programming can keep your code aligned further to the left and the state that you actually need in each function is much more explicit, narrowly-defined, and easier to refactor. For smallish things closures are great but I've found that when smallish closures grow into less smallish chunks of code it's usually much cleaner to refactor them into prototype classes.

For a good example of how closures can be very confusing and unreadable (and why too many wall-of-text inline comments can really obscure the intent of some code) there's the json source.

You don't need to use bind to get the benefits of flexible callers, you can use this trick: https://github.com/substack/frequency-viewer/blob/master/index.js#L18

inheritance is mostly a poor approach except when it isn't. A good example of when inheritance is a good idea is when you build an interface that conforms to a platform primitives such as streams or event emitters.

Every feature has some domain of applicability and every programmer has their own standard approaches for organizing functionality. It's too blunt to decry an approach universally without recognizing its appropriate domain of application. Sometimes that domain is very narrow but in the case of this and class prototypes, the approach can be useful and in some contexts more ideal than the alternatives.

To address more concretely your example, you probably shouldn't do this.log=... in your constructor. Instead you could write:

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

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

and then you avoid most of the downsides you talked about while getting 2 new upsides: removing one level of indentation and the ability to inherit when you really need to. For example to turn that example into an event emitter, you can just do:

var inherits = require('inherits');
var EventEmitter = require('events').EventEmitter;

inherits(CountingLogger, EventEmitter);

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

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

versus the alternative in a closure:

var EventEmitter = require('events').EventEmitter;

function CountingLogger() {
  var self = new EventEmitter;
  var count = 0;
  self.log = function (msg) {
    console.log(count, msg);
  };
  return self;
}

I usually prefer the former prototype-based approach but it's really much more of an unimportant stylistic thing than some intrinsic airtight argument where one approach is always better than the other, although there are some cases in hot loops where for performance prototypes are more favorable. Everything is trade-offs.

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.