Can multiple closures to add variables to a function's scope be simplified?

Question

I'm attempting to pass two variables to a filereader callback. Essentially I'm looping over files and displaying thumbnails of them; to accomplish this, I need two variables (in this case, parent and canvas). The code below works, but I want to be sure my future self won't hate my present self.

I assume that passing two variables into the scope of a callback function without overwriting this is a fairly common case.

This code is using a namespace (parent), which could be avoided, but I would still like to know how to pass two variables into a callback function.

reader.onload = (function (parent) { // even funkier closure to access the parent, the canvas, and the file
    return function(canvas) { // funky closure to be able access the canvas and file
        return function(r) {
            canvas.sourceImage.src = r.target.result;
            parent.functions.render(canvas);
            parent.functions.preview(canvas, canvas.sourceImage.width/10, canvas.sourceImage.height/10);
        };
    }(parent.canvases[j]);
})(parent);

reader.readAsDataURL(file);

---

Per comments, here's the full code. I'm not particularly looking for a code review (there's a stack for that!), but any feedback is appreciated.

(function () {
    this.readFiles = function (parent) {
        var files = parent.fileInput.files;
        var j = parent.canvases.length; // so we don't overwrite old previews
        for (var  i = 0; i < files.length; i++, j++) {
            var file = files[i];
            var reader = new FileReader;

            var canvas = document.createElement('canvas');
            canvas.id = "previewCanvas" + j;
            canvas.className = parent.canvasClasses;
            canvas.width = 100;
            canvas.height = 100;
            parent.previewWrapper.appendChild(canvas);

            /* Initialize Canvases for each file */
            parent.canvases[j] = this.create(document.getElementById('previewCanvas' + j));

            reader.onload = (function (parent) { // even funkier closure to access the parent, the canvas, and the file
                return function(canvas) { // funky closure to be able access the file
                    return function(r) {
                        canvas.sourceImage.src = r.target.result;
                        parent.functions.render(canvas);
                        parent.functions.preview(canvas, canvas.sourceImage.width/10, canvas.sourceImage.height/10);
                    };
                }(parent.canvases[j]);
            })(parent);

            reader.readAsDataURL(file);
        }
    }
}).apply(Canvas.functions);

Answer 1

Basically you can get rid most of the closures and a few other bits of code too.

This should work, barring any bugs I've introduced... And with the two bugs that you noted now fixed, thanks for catching them! :-)

function forEach( array, callback ) {
    Array.prototype.forEach.call( array, callback );
}

Canvas.functions.readFiles = function( parent ) {
    forEach( parent.fileInput.files, function( file ) {
        var reader = new FileReader;

        var canvas = document.createElement( 'canvas' );
        canvas.id = 'previewCanvas' + parent.canvases.length;
        canvas.className = parent.canvasClasses;
        canvas.width = 100;
        canvas.height = 100;
        parent.previewWrapper.appendChild( canvas );

        var fileCanvas = Canvas.functions.create( canvas );
        parent.canvases.push( fileCanvas );

        reader.onload = function() {
            fileCanvas.sourceImage.src = reader.result;
            parent.functions.render( fileCanvas );
            parent.functions.preview(
                fileCanvas,
                fileCanvas.sourceImage.width / 10,
                fileCanvas.sourceImage.height / 10
            );
        };
        reader.readAsDataURL( file );
    }
};

A few notes:

You don't need the outermost IIFE that turns Canvas.functions into this. Just use Canvas.functions directly. That also eliminates any chance of this being incorrect inside a callback (doesn't appear that would be a problem here, but it helps keep things simple).

You don't need the j variable. parent.canvases.length is always the same value as j.

If you use .forEach() instead of the for loop, you get a closure on all the local variables declared inside the .forEach() callback. This is really the only closure you need.

OTOH, as you noted, an "array-like" object like FileReader may not have a .forEach() method. There are a couple of ways ways to address this. One is to write an ordinary for loop and call a function in it, e.g.

var files = parent.fileInput.files;
for( var i = 0;  i < files.length;  i++ ) {
    doFile( files[i] );
}

function doFile( file ) {
    ...
}

This could be done with an inline function expresion, but I like using a separate named function statement for this kind of use - it just seems a little more clear. (The one caveat here would be to not put the function statement itself inside a nested block like an if or for block - browsers do not handle that consistently - it needs to be directly inside the outer function.

Or, use Array.prototype.forEach.call() to let you use forEach on the array-like object. I used this approach in the updated code above, by defining a standalone forEach() function. You could just put the Array.prototype.forEach.call() directly inline, but I think it's cleaner to wrap it in a function. Of course if you're using a library such as Underscore or LoDash there are convenient iterators you can use like _.each().

You don't need to use document.getElementById() to get a reference to the current canvas, because you already have it in the canvas variable.

You don't need either of the nested closures inside the onload() callback. That function already has access to the necessary variables.

The code appears to use the name canvas for two very different things. First is the actual canvas element that you create at the top, second is the return value from Canvas.functions.create(canvas). I renamed the second one as fileCanvas to avoid confusion. You can probably come up with a better name, just don't call it canvas.

In the onload callback, r.target is the same object as reader, so you can just use it directly. If you do use a parameter in an event handler like this, I recommend calling it event (or e if you want a short name) since that is more customary.

Answer 2

Although @MichaelGeary mostly answered my question by restructuring the code, the answer to the title question is that the outer function is executed immediately, and has access to the current variables:

reader.onload = (function (parent) { 

   var canvas = parent.canvases[j];
   // put more variables ad nauseum here. 
   // They will be passed to the return function without being changed.

   return function(r) {
        canvas.sourceImage.src = r.target.result;
        parent.functions.render(canvas);
        parent.functions.preview(canvas, [...]);
        };
})(parent);

This way you can use counters and other variables likely to change if needed inside of the callback.