Assigning prototype methods *inside* the constructor function - why not?

Question

Stylistically, I prefer this structure:

var Filter = function( category, value ){
  this.category = category;
  this.value = value;

  // product is a JSON object
  Filter.prototype.checkProduct = function( product ){
    // run some checks
    return is_match;
  }

};

To this structure:

var Filter = function( category, value ){
  this.category = category;
  this.value = value;
};// var Filter = function(){...}

Filter.prototype.checkProduct = function( product ){
  // run some checks
  return is_match;
}

Functionally, are there any drawbacks to structuring my code this way? Will adding a prototypical method to a prototype object inside the constructor function's body (i.e. before the constructor function's expression statement closes) cause unexpected scoping issues?

I've used the first structure before with success, but I want to make sure I'm not setting myself for a debugging headache, or causing a fellow developer grief and aggravation due to bad coding practices.

Answer 1

Functionally, are there any drawbacks to structuring my code this way? Will adding a prototypical method to a prototype object inside the constructor function's body (i.e. before the constructor function's expression statement closes) cause unexpected scoping issues?

Yes, there are drawbacks and unexpected scoping issues.

1. Assigning the prototype over and over to a locally defined function, both repeats that assignment and creates a new function object each time. The earlier assignments will be garbage collected since they are no longer referenced, but it's unnecessary work in both runtime execution of the constructor and in terms of garbage collection compared to the second code block.

2. There are unexpected scoping issues in some circumstances. See the Counter example at the end of my answer for an explicit example. If you refer to a local variable of the constructor from the prototype method, then your first example creates a potentially nasty bug in your code.

There are some other (more minor) differences. Your first scheme prohibits the use of the prototype outside the constructor as in:

Filter.prototype.checkProduct.apply(someFilterLikeObject, ...)

And, of course, if someone used:

Object.create(Filter.prototype) 

without running the Filter constructor, that would also create a different result which is probably not as likely since it's reasonable to expect that something that uses the Filter prototype should run the Filter constructor in order to achieve expected results.

---

From a run-time performance point of view (performance of calling methods on the object), you would be better off with this:

var Filter = function( category, value ){
  this.category = category;
  this.value = value;

  // product is a JSON object
  this.checkProduct = function( product ){
    // run some checks
    return is_match;
  }

};

There are some Javascript "experts" who claim that the memory savings of using the prototype is no longer needed (I watched a video lecture about that a few days ago) so it's time to start using the better performance of methods directly on the object rather than the prototype. I don't know if I'm ready to advocate that myself yet, but it was an interesting point to think about.

---

The biggest disadvantage of your first method I can think of is that it's really, really easy to make a nasty programming mistake. If you happen to think you can take advantage of the fact that the prototype method can now see local variables of the constructor, you will quickly shoot yourself in the foot as soon as you have more than one instance of your object. Imagine this circumstance:

var Counter = function(initialValue){
  var value = initialValue;

  // product is a JSON object
  Counter.prototype.get = function() {
      return value++;
  }

};

var c1 = new Counter(0);
var c2 = new Counter(10);
console.log(c1.get());    // outputs 10, should output 0

Demonstration of the problem: http://jsfiddle.net/jfriend00/c7natr3d/

This is because, while it looks like the get method forms a closure and has access to the instance variables that are local variables of the constructor, it doesn't work that way in practice. Because all instances share the same prototype object, each new instance of the Counter object creates a new instance of the get function (which has access to the constructor local variables of the just created instance) and assigns it to the prototype, so now all instances have a get method that accesses the local variables of the constructor of the last instance created. It's a programming disaster as this is likely never what was intended and could easily be a head scratcher to figure out what went wrong and why.

Answer 2

While the other answers have focused on the things that are wrong with assigning to the prototype from inside the constructor, I'll focus on your first statement:

Stylistically, I prefer this structure

Probably you like the clean encapsulation that this notation offers - everything that belongs to the class is properly "scoped" to it by the {} block. (of course, the fallacy is that it is scoped to each run of the constructor function).

I suggest you take at the (revealing) module patterns that JavaScript offers. You get a much more explicit structure, standalone constructor declaration, class-scoped private variables, and everything properly encapsulated in a block:

var Filter = (function() {

    function Filter(category, value) { // the constructor
        this.category = category;
        this.value = value;
    }

    // product is a JSON object
    Filter.prototype.checkProduct = function(product) {
        // run some checks
        return is_match;
    };

    return Filter;
}());

Answer 3

The first example code kind of misses the purpose of the prototype. You will be recreating checkProduct method for each instance. While it will be defined only on the prototype, and will not consume memory for each instance, it will still take time.

If you wish to encapsulate the class you can check for the method's existence before stating the checkProduct method:

if(!Filter.prototype.checkProduct) {
  Filter.prototype.checkProduct = function( product ){
    // run some checks
    return is_match;
  }
}

There is one more thing you should consider. That anonymous function's closure now has access to all variables inside the constructor, so it might be tempting to access them, but that will lead you down a rabbit hole, as that function will only be privy to a single instance's closure. In your example it will be the last instance, and in my example it will be the first.

Answer 4

Biggest disadvantage of your code is closing possibility to override your methods.

If I write:

Filter.prototype.checkProduct = function( product ){
    // run some checks
    return different_result;
}

var a = new Filter(p1,p2);

a.checkProduct(product);

The result will be different than expected as original function will be called, not my.

Answer 5

In first example Filter prototype is not filled with functions until Filter is invoked at least once. What if somebody tries to inherit Filter prototypically? Using either nodejs'

function ExtendedFilter() {};
util.inherit(ExtendedFilter, Filter);

or Object.create:

function ExtendedFilter() {};
ExtendedFilter.prototype = Object.create(Filter.prototype);

always ends up with empty prototype in prototype chain if forgot or didn't know to invoke Filter first.

Answer 6

Just FYI, you cannot do this safely either:

 function Constr(){

    const privateVar = 'this var is private';

    this.__proto__.getPrivateVar = function(){
       return privateVar;
    };

 }

the reason is because Constr.prototype === this.__proto__, so you will have the same misbehavior.