3
\$\begingroup\$

I've been using this syntax alot recently to keep my class in scope of nested functions. It seems to work well in small to moderately sized class but I'm starting to wonder how efficient it actually is and if there is a better way to handle this. Any suggestions would be really useful.

myClass.prototype.myMethod = function() {
    var that = this; // Without taking the size of the class into consideration 
                     // does this create an unecessarily high load?
    function callback_one(data) {
        that.myOtherMethod();
    }

    function callback_two(data) {
        that.myErrorMethod();
    }

    $.post(this.getRequest(), this.getUrl())
        .success(function(data) {
            callback_one(data);
            // Can also use that.anotherMethod() here
        })
        .error(function(data) {
            callback_two(data);
            // Can also use that.anotherMethod() here
        });
}
\$\endgroup\$

closed as off-topic by Jamal Nov 8 '16 at 22:34

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

4
\$\begingroup\$

Your code is fine (assuming your nested functions are merely proxies to the class methods), I would suggest if the only thing you are doing in the anonymous functions is calling the callbacks then you should just pass them in:

$.post(this.getRequest(), this.getUrl()).success(callback_one).error(callback_two);

There is nothing wrong with the use of that in this code.

Edit (after reading Esailija's answer)

You could also do the proxies mentioned in the Esailija's answer inline like so:

myClass.prototype.myMethod = function () {
    $.post(this.getRequest(), this.getUrl())
        .success($.proxy(this.myOtherMethod, this))
        .error($.proxy(this.myErrorMethod, this));
}

Essentially that is what you were doing with the nested functions; this just tightens the code by moving the proxy creation into a utility method.

\$\endgroup\$
  • \$\begingroup\$ Thanks Bill that's put my mind at ease, after writing out two large classes I feared I'd dumped myself in it by using this, I mean that... \$\endgroup\$ – David Barker Jul 5 '12 at 18:58
  • \$\begingroup\$ You don't want to do the proxying in the method, it can be done in the constructor once per instance creation instead of once per method call. It also ensures that all proxying is done in one place and the object is fully initialized after construction (can start passing handlers without losing context) \$\endgroup\$ – Esailija Jul 5 '12 at 19:13
  • \$\begingroup\$ @Esailija: A class is not an atomic unit in JS, but a function is. If I am going to pass a function off to an event, I expect the context to change. I shouldn't have to go look at a third function to realize that some magic happened so that the context didn't change. \$\endgroup\$ – Bill Barry Jul 5 '12 at 19:26
  • \$\begingroup\$ I can see arguments for both of the answers you guys gave, I don't see any problem with putting the code in the constructor to maintain the scope of a method if you know you've done it. On the other hand I can see that as a third party looking at it, if embedded it could cause more than a few headaches. Thanks for both your answers, you both gave me some piece of mind. \$\endgroup\$ – David Barker Jul 5 '12 at 19:34
  • \$\begingroup\$ @Bill Barry What did you mean exactly by 'assuming your nested functions are merely proxies to the class methods', what if they weren't just the above and also call other methods etc.? \$\endgroup\$ – David Barker Jul 5 '12 at 21:00
3
\$\begingroup\$

Yes, you can avoid creating functions inside functions entirely and you are correct it would be unmaintainable in the long term.

function MyClass() {
    //Proxy methods in constructor, so that the instance will have fixed context in the callbacks
    this.ajaxSucceeded = $.proxy( this.ajaxSucceeded, this );
    this.ajaxFailed = $.proxy( this.ajaxFailed, this );
}

MyClass.prototype.myMethod = function() {
    $.post(this.getRequest(), this.getUrl())
        .success( this.ajaxSucceeded )
        .error( this.ajaxFailed );
};

MyClass.prototype.ajaxSucceeded = function( data ) {
    //this.myOtherMethod(); <-- is possibly redundant now
};

MyClass.prototype.ajaxFailed = function( xhr, status ) {
    //this.myErrorMethod(); <-- is possibly redundant now
};
\$\endgroup\$
  • \$\begingroup\$ This is a cool solution, This is my first encounter with .proxy and it looks like it would clean my code up loads \$\endgroup\$ – David Barker Jul 5 '12 at 19:00
  • 1
    \$\begingroup\$ @DavidBarker the native version (but only modern browsers support it) is Function#bind \$\endgroup\$ – Esailija Jul 5 '12 at 19:02
  • \$\begingroup\$ I get why this works, but it looks wrong to me. Inside myMethod (without knowing that the lines are defined as such in the constructor) I would expect this to be changed when it actually calls the ajaxSucceeded function (it looks wrong because of how I expect success to work). Also, overwriting prototype attached functions with instance ones strikes me as a very strange practice. \$\endgroup\$ – Bill Barry Jul 5 '12 at 19:19
  • \$\begingroup\$ @BillBarry it doesn't overwrite the prototype. When you set a property of an object, it's always on that object. When you get a property of an object, it might be gotten from that object's prototype. But you never set to prototype (unless you set this.__proto__ of course). Also this works, even if it looks strange. You can test it easily :P. Basically the prototype method is a generic blueprint and each instance gets their own proxied version of it that is fixed to the context of that instance. \$\endgroup\$ – Esailija Jul 5 '12 at 19:21
  • \$\begingroup\$ I know it works; I am saying if I am reading myMethod here only I would be expecting that it wouldn't. I am also saying that I find the act of replacing something on the prototype with something having the same name in the constructor (overwriting a prototype attached function with an instance attached function) to look strange. \$\endgroup\$ – Bill Barry Jul 5 '12 at 19:30

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