

Extending the Revealing Module Pattern – Why is this a bad idea? - invalid_arg
http://invalid-arg.github.io/2013/04/13/extending-the-revealing-module-pattern.html

======
tantalor
The revealing module pattern is an antipattern. Instead, just document which
of your methods are private or protected,

    
    
      /** @private */
      MyClass.prototype.foo_ = function() {}

~~~
mistercow
I have to agree with you, if nothing else because existing debugging tools
just aren't very good at dealing with the privacy-through-closures pattern. In
many cases, it's essentially impossible to inspect the state of a variable
that is made private this way, and it's not clear how you would even design a
debugger that gave a reasonable interface for doing so.

~~~
ratbeard
Its annoying, but certainly possible to inspect the state of a local variable
inside of a closure. Set a breakpoint inside of one of the exported functions,
call it, and you have access to all the local variables within the closure.

I do dislike this pattern as I find the code is less structured, harder to
debug, and less performant than attaching functions on the prototype. This
outweighs the benefits of encapsulation to me.

~~~
doomslice
I have never, ever come across a scenario where assigning to the object
instead of the prototype has made any meaningful impact on performance.

The only scenario I can even think of where it would maybe possibly matter
would be a tight loop where you are doing little else except just accessing
properties.

~~~
ratbeard
I can imagine it having an impact on a graphics app where a `Point` class
might be instantiated thousands of times, or even if the Backbone.Model
constructor were hundreds of lines long to attach all the public methods. It
would be weird to me to have to spend a few seconds of thought on a per-object
basis to think how I should structure the code based on how often I think it
will be instantiated.

But yes, I also have not been bitten by this. Its just something I've read
about how the v8 engine works (and makes sense intuitively). I agree you
shouldn't base coding style decisions soley on hearing one way is faster, but
I think its a minor point and worth noting.

Edit: a link! <https://news.ycombinator.com/item?id=2991904>

------
gibbitz
I do understand the difficulty of testing private code in tools like jasmine
when you use private variables based on publicly unreachable scopes. When I
have to do this, I add a debug object with references to the code. Generally,
this code should affect publicly accessible methods and those are more
important to test. If they fail, decent in-browser tools like breakpoints and
watch lists should reveal the error.

I'm not completely ready to call this an anti-pattern. There's no reason to
expose local methods of an Object to the parent scope if they aren't needed.
I'm sure that in some cases this could cause the memory footprint of an Object
to be larger than desired by preventing garbage collection. Anyway, I would
have factored your code more like this:

    
    
      var PseudoClass = function(_args){
          // guarantee that "this" is what we expect
          if( this === window ){ // use global for non-browser    implementations
             return new PseudoClass(_args);
          }
          // private variables are local
          var private = function() {;
                 this.value = 2; 
              };
          // public variables are on prototype 
          this.constructor.prototype.value = 1; // I'd prolly use a local var and a getter for this
          this.constructor.prototype.public = function(){
              private.call(this); // force scope for private methods
          };
          // testing access (only added when necessary)
          this.constructor.prototype.privateMembers = {
              "private":function(){ private.call(this) }
          };
        };
    

There's no "self" here, but there are two pieces of insurance around scope.
First is the condition that forces the "new" keyword. (This could be written
to test against instanceof the function name, but generally problems occur
when users forget new, not when they force the scope to be wrong. Last I
checked, this is how jQuery tests it too.) Second is calling the private
methods with the call() method of the function, allowing us to keep the scope
from defaulting to global/window. Sure, you are forced to put these in to
avoid errors, but this is programming. If you would like it to throw an error
when this === window within private(), just add it. I didn't because a) I'm
lazy and b) once the PseudoClass private methods are all called, I would end
up removing the un-thrown errors and the privateMembers property before
compressing for staging/production to keep size down.

Using a variable like "self" is quite clear in this context and I've seen it
done thousands of times. I just wanted to show that using "this" is still
possible.

Javascript is a funny language. I often find myself wondering if this was the
intended structure of the language as it takes advantage of many of the key
elements (closure scopes & prototype) while abstractly appearing like simple
Java (or C#). Sure in a perfect world, the wrangling of "this" wouldn't need
to be done. I'd say the unpredictable value of "this" in different contexts is
the biggest fail of Javascript. I programmed ActionScript (including AS1) for
many years and this (pun intended) is the biggest mindf*ck I've come across
when regarding them in comparison (considering they're both based on ECMA
script). Hope there was something helpful to you in this.

------
RyanZAG
The pattern also seems like it would be an impediment to performance. The JIT
would have to do a lot more work to optimize the function, as well as parse
code that isn't really required. So even if the end-result machine code after
JIT was performed was the same, the compiler itself would have a much harder
time getting there. This can be an issue on mobile devices that are CPU
constrained where you want the page to load as fast as possible.

------
doomslice
I don't really see the need for the extend at all. In fact if you don't use
it, it will work the same as example 1 and you'll still have access to self.

Or you can just do this.foo = foo; this.value = value; at the end instead of
returning an Object.

~~~
platz
Yes extend is unnecessary in this case. The key was saving a reference to
self.

------
emehrkay
The problem is the foo function not being apart of the returned object.
Something like this is a very easy solution without the need to use
call/apply/jquery.extend

<http://jsfiddle.net/ABD3C/>

------
jonny_eh
I don't see why this is "bad" other than depending on jQuery to do something
not directly related the browser. That could easily be fixed by swapping
jQuery out for Underscore, since it would then work in browser and in Node.

