6
\$\begingroup\$

I created a function which has, I believe, truly private variables. It's not quite production ready for several reasons:

  • It's using Object.defineProperty -- I can deal with this
  • It is re-defining every function in the prototype by doing eval( fn.toString() ) -- I do not like this
  • I'm re-defining the prototypes, hence doing-away with the advantage of prototypes! -- deal breaker

Anyway, I'm putting this out there for critique / suggestions / recommendations for how to fix the above points. I'm not sure I'll ever really do anything with it, but I find it to be an interesting problem:

function foo(){
    this.definePrivateProperties('foo', 'bar');
}

foo.prototype.setFoo = function(x){
    this.foo = x;
};

foo.prototype.getFoo = function(){
    return this.foo;
};

foo.prototype.definePrivateProperties = function(){
    var   self = this,
          args = arguments,
      validFns = {},
         proto = this.__proto__;

    // replace all functions in prototype with new ones!
    // set a random key on each new function
    //   (not 100% secure, but close enough for now)
    //   this ensures that the function is unique to -this- object
    for(var fn in proto){
        this[ fn ] = eval( '(' + proto[fn] + ')' );
        validFns[ this[ fn ].key = Math.random() ] = 1;
    }

    for(var i=0, l=args.length; i<l; i++){
        (function(){
            var val;

            Object.defineProperty(self, args[i], {
                get          : function getter(){
                    if(validFns[ arguments.callee.caller.key ])
                        return val;
                },
                set          : function setter(v){
                    if(validFns[ arguments.callee.caller.key ])
                        val = v;
                },
                configurable : true,
                enumerable   : false
            });
        })();
    }
};

> var x = new foo();
> x.setFoo(42);
> x.foo = 20;
20
> x.foo;
undefined
> x.getFoo();
42
\$\endgroup\$
  • 1
    \$\begingroup\$ You will likely get more informed comments if you ask on the Usenet forum comp.lang.javascript, which can be accessed using a newsreader (referable), or using Google Groups. \$\endgroup\$ – RobG Jul 7 '11 at 1:19
  • 1
    \$\begingroup\$ Why not use the closure mechanism for creating truly private JS variables as described here: javascript.crockford.com/private.html. \$\endgroup\$ – jfriend00 Nov 3 '11 at 16:28
6
\$\begingroup\$

As for the code:

  • The property getter and setter should probably throw an exception if the check against the caller failed. If you attempt to set it from any function besides one defined on the prototype, you don't want it to silently fail -- if it does, you'll be wasting lots of time trying to figure out why your changes aren't sticking. (Particularly in light of other issues below.)

  • If you're going to assign a different key to each member function, you might consider making validFns[key] be the function with that key, rather than just '1', so you can check it against the caller. (Ensures the key doesn't ever accidentally match.)

  • You're setting keys on each thing you copy from the prototype, regardless of whether it's a function. Only functions need keys; other stuff (well, anything really, but particularly non-function members) might have its own purpose for key.

  • The way the access checks are now, only member functions can use them. Though that may be the intent, it is way more crippling than it sounds.

Observe:

foo.prototype.doStuff = function() {
    var self = this;
    $('input.some_selector').each(function() {
        self.foo += parseInt(this.value);
    });
};

We're in a private function, which should have access to foo. But since the actual access is within an anonymous function, and not doStuff, it's not allowed. In order to fix this (and fix it you should), your access check would need to go down the call stack (check the caller, then the caller's caller, and so on) til it finds a function with a key. Recursive functions seem to cause such a search massive amounts of trouble, though; a depth limit might help with that.

  • definePrivateProperties can be run a second time. And a third, fourth, etc. And each time you call it, it obliterates the previous property's value in order to add the new one. Read: external code can mess up your private variables.

Say:

foo.prototype.mangleFoo = function() { this.foo = 100; };
x.definePrivateProperties('foo');
delete foo.prototype.mangleFoo;
x.mangleFoo();

(BTW, see what we just did there? We can add a function and give it access to the "private" properties pretty much at will.) In order to prevent that...i dunno. redefine definePrivateProperties after it's run once?

In contrast, the standard method (hiding the variable away in a closure) is not prone to such hackery.

\$\endgroup\$
2
\$\begingroup\$

It looks like you can get rid of the eval

this[ fn ] = eval( '(' + proto[fn] + ')' );

by replacing it with

this[fn] = (proto[fn]);

Here's a jsfiddle showing this change logs the same results as your sample.

\$\endgroup\$
  • \$\begingroup\$ Actually that won't work. The reason I eval'd the function was so that I could get a new function with the same code that I could then set .key on (the next line). This .key has to be bound to a unique instance of the function otherwise there is confusion between class instances and everything falls apart... So what you did will work with one instance of the class, but not two. \$\endgroup\$ – zyklus Aug 24 '11 at 20:09
0
\$\begingroup\$
this[ fn ] = eval( '(' + proto[fn] + ')' );

will lose the relationship between the function and any variables it closes over. Better to do

this[ fn ] = (function (f) { return f.apply(this, arguments); })(proto[fn]);

You're also using non-standard constructs like __proto__. Maybe it would be better to use getPrototypeOf

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.