views:

296

answers:

1

I have this code, I used this on some of JavaScript components I built for a project. Now I want to know if is there a memory leak on the following code.

Which of the options is most appropriate, A or B or is there a better way?

var component = function(){
    var self = this; //A - not sure there's a leak here

    this.foo = function(){
        //var self = this; //B. I can do this but I want to use self in other method as well
        var dom = getElementById('someid');
        dom.onclick = function(){
            self.foo2(); // here I used the self reference
            //i cannot use this here, because it refer to dom
        }
    }

    this.foo2 = function(){
        var dom = getElementById('someid');
        dom.onclick = function(){
            self.foo2(); //here I used the self reference
            //i cannot use this here, because it refer to dom
        }
    }
};

// some usage

var c1 = new component();
c1.foo();
+6  A: 

Memory consumption

The problem with that approach is that all of the code for all of your methods will be duplicated for each and every instance, which can turn into a big memory drain (barring nifty optimisations around execution contexts that you can't rely on). The advantage to what you've done there is that the click handler directly calls the method you want it to call.

The usual thing is to have the methods shared amongst instances by putting them on the prototype, and for instances to have small wrapper functions built in a well-contained closure context (e.g., one that doesn't close over extraneous data) -- usually created by a helper function for that reason -- that set up the call to the instance. That way, only small amounts of code are duplicated for each instance, the majority of it is shared. The cost is that each click requires a call to a function that then turns around and calls another function, but frankly that overhead isn't really an issue except in very tight loops (which click handlers aren't), whereas memory consumption really can be an issue in today's webapps.

Setting up the functions on the prototype is well-covered elsewhere and usually handled through helpers that let you be clearer and more concise than the raw JavaScript. For examples of wrapper builders in well-contained closure contexts, check out the Function#bind implementation in Prototype (or similar functions in any of several other JavaScript libraries like MooTools, the Closure Library, etc.)

The basics look like this, but I wouldn't actually do it this way:

var component = function() {
    this.boundFoo = bind(this, foo); // Remove this if you never use it as a handler
    this.boundFoo2 = bind(this, foo2);
};
component.prototype.foo = function() {
    var dom = getElementById('someid');
    dom.onclick = this.boundFoo2;
};
// (Isn't this exactly what foo did?)
component.prototype.foo2 = function() {
    var dom = getElementById('someid');
    dom.onclick = this.boundFoo2;
};
function bind(context, func) {
    return function() {
        func.apply(context, arguments);
    };
}

Note how bind accepts the context and the function, and returns a new function that will call the given function with that context.

You could also bind the functions at the point you're assigning them rather than setting them as properties on the instance, but if you're going to reuse them (as you seem to be above), then keeping a copy makes that possible.

There are issues with the above (all of the functions are anonymous, which means your tools can't help you) but without getting into object helper functions, that's the basic idea.

Memory Leaks

"Crescent Fresh" pointed out I didn't really address memory leaks originally. One important aspect to address is that with some browsers (IE and derivatives, mostly), it's important to unhook your event handler when you're done with it (for instance, when leaving the page). And so both of your onclick assignments could be memory leaks on some browsers if you don't clear them later. This is because the browsers involved don't handle cleaning up circular references between DOM elements and JavaScript objects when neither is still referenced (e.g., they reference each other but nothing else references either of them). You have to break the link between the DOM element and the JavaScript function to ensure that both are cleaned up. Libaries like Prototype do that for you when the page is unloaded if you use their methods for attaching event handlers; I don't know other libs well enough to comment on whether they do.

Other Notes

Somewhat OT, but:

  1. Also look into the concept of "event delegation" -- click handlers are a great place to use delegation, since they bubble. You may find you only need a couple of handlers at the container level, rather than hundreds at the element level.
  2. Rather than assigning to the onclick property of the element, which is called the "DOM0" style of setting event handlers, consider using the newer (late 90's) mechanisms for doing this: addEventListener on standards-compliant browsers and attachEvent on IE. One of the big improvements doing things this way is that multiple handlers can be set for the same event on the same element, whereas with DOM0 handlers, assigning a new one clobbers the old one if there is one there.
T.J. Crowder
Some good points. Couple general comments: your answer doesn't actually address memory leaks. Memory consumption yes, but leaks? (or did I miss something implied?) WRT memory consumption one caveat you don't mention is that it would take instantiating thousands of these things to notice any remarkable memory usage over the `prototyped` version (potentially consumption tied to leaks). From the *looks* of the OPs code (`getElementById`, `onclick=`), there'd be 1 of these things around. Finally, jQuery is actually "missing" a `bind` equivalent. Perhaps in 1.4 it'll come though, I'm not sure.
Crescent Fresh
@Crescent Fresh: Good point about leaks; fixed. Thanks for the jQuery info. To me, it's not at all clear the OP won't be attaching these component objects to dozens or hundreds of elements on the page, and/or applying this pattern to other objects he'll be creating in large numbers. The pattern being suspect, probably best to point it out.
T.J. Crowder