Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

After having spent a month or two trying to learn JavaScript, especially functional programming, async, and closures, I finally get it. But I don't know if I'm writing elegant code... Specifically, I'm not sure if I'm creating too many closures, because I don't entirely grasp when contexts remain and when they're marked for garbage collection.

You can probably tell from the example that I'm using node.js -- basically, this is just a piece of test code that connects to a database, gets 20,000 records asynchronously with individual SELECT queries (I'm just doing some profiling, don't worry), and manipulates the data. At the end, it tells me how long it took and closes the MySQL connection.

In order to get a final timestamp and close the connection, I needed something like the async library to keep track of all the various asynchronous functions that are being spawned. It takes all my queries as an array of functions (using a factory called makeQuery()) and then runs a callback to do the cleanup.

[EDIT] I just realised I didn't ask any questions. Here they are:

  1. Am I creating a huge amount of contexts by creating and returning the function doQuery() which uses the argument id in the makeQuery() function, thereby causing 20,000 contexts to persist until async.parallel() is run?

  2. Is the factory function makeQuery() a good and tidy way to create this array of doQuery() functions? Can you suggest any better way to do it?

I think that's it, but if you notice any other issues, let me know!

var mysql = require('mysql');
var radix64 = require('./lib/radix64');
var async = require('async');

var client = mysql.createClient({user: 'root', password: '12345678'});
client.query('USE nodetest');

var startTime = (new Date()).getTime(); // poor man's profiling
var queries = []; // this will be fed to async.parallel() later

var makeQuery = function makeQuery (id) { // factory function to create the queries
    return function doQuery (cb) {
        client.query('SELECT * FROM urls WHERE id='+id,
            function logResults (e, results, fields) {
                results = results[0];
                results.key = radix64.fromNumber(results.id); // yep, I'm converting the
                                                              // number into base-64 notation
                cb(); // if I wanted to do something useful at the end, I would have
                      // called cb(results) instead, which compiles an array of results
                      // to be accessed by the final callback
            }
        );
    };
};

for (i = 1001; i <= 20000; i ++) { // build the list of tasks to be done in parallel
    queries.push(makeQuery(i));
}

// run those tasks
async.parallel(queries, function finished () { // clean up and get the elapsed time
    console.log('done');
    client.end();
    console.log(((new Date()).getTime() - startTime) / 1000);
});

For those of you unfamiliar with async, it's a userland module built for node.js and the browser. Each function in the array that gets passed to async.parallel() is obliged to take a callback and then run that callback once it's done, in order to let async.parallel() know it's done.

share|improve this question
    
@Raynos yes. Oops. Just a test server running on 127.0.0.1, but still... –  Paul d'Aoust Nov 16 '11 at 16:22
    
@Raynos would you be able to delete your comment, to hide the evidence? I removed the password in my code sample. –  Paul d'Aoust Nov 16 '11 at 16:34
    
There's a change log on the question, you know. –  Raynos Nov 16 '11 at 16:55
    
oh yeah, you're right. Dang. –  Paul d'Aoust Nov 16 '11 at 17:10

2 Answers 2

up vote 2 down vote accepted

var makeQuery = function makeQuery (id) {

There is no need to make makeQuery a local variable, just use a function declaration.

results = results[0];
results.key = radix64.fromNumber(results.id);

Your just augmenting the object, your not doing anything with it.

Other then that, your not creating new functions in a loop, your using a function constructor. This correct.

Your running all 19000 queries in parallel rather then in waterfall, which is also correct.

Edit:

Question 1:

You need to create 20000 contexts. Because there are 20000 values of i. Worrying about there being 20000 functions is a micro optimisation. v8 optimizes the hell out of your code.

It actually splits your functions into a hidden class seperate from the closure context, and in my memory you simply have 20000 values of i and one value for the function.

Just to remind you of rule 1.

Never underestimate V8

Question 2:

makeQuery as a factory is the correct pattern to use.

The only other optimisation you can do is to write a real SQL query rather then 20000 dummy ones. But I'll ignore that because it's a dummy example.

share|improve this answer
    
Re: having makeQuery a local variable, I guess you're right... I've just been too Crockfordised, that's all :) And in regards to augmenting results and then totally throwing it away, I know... just doing a speed test, so I don't want to actually use it. Thanks for the confirmation that using a constructor is correct... Also, I updated my question with actual explicit questions! My big concern is whether the constructor function is creating 20,000 contexts on account of the closure in which the returned function needs the id argument. Oh, and whether that's a problem at all. –  Paul d'Aoust Nov 16 '11 at 16:32
    
Yep, it's a dummy example. Trying to simulate the real-world use case, in which 20,000 people might indeed hit the server at once with their tiny little queries. Glad to know V8 won't mind about all my contexts. So when you say it splits that returned function out into its own, do you know if it would still do that if it were an anonymous function? Another Crockford-instilled anxiety. –  Paul d'Aoust Nov 16 '11 at 16:37
    
@Pauld'Aoust functions are functions. There's no difference –  Raynos Nov 16 '11 at 16:54
    
Oh, okay. Crockford mentioned something about naming your functions so that you get 20,000 references to one function rather than 20,000 anonymous functions in the heap. Perhaps V8 doesn't do it that way, or else I didn't understand him correctly. At any rate, I guess it's good practice to name your callbacks so you get something to look at in your stack traces. –  Paul d'Aoust Nov 16 '11 at 17:14
    
@Pauld'Aoust the point is you have one named function nameQuery and 20000 anonymous functions (the functions returned from the nameQuery). –  Raynos Nov 16 '11 at 17:22

When in doubt, go with restricting the closure context to the enclosing function scope:

function makeLog(cb) {
    return function (e, results, fields) { // logResults
        results = results[0];
        results.key = radix64.fromNumber(results.id); // yep, I'm converting the
                                                      // number into base-64 notation
        cb(); // if I wanted to do something useful at the end, I would have
              // called cb(results) instead, which compiles an array of results
              // to be accessed by the final callback
    };
}

function makeQuery(id, aClient) { // factory function to create the queries
    return function (cb) { // doQuery
        aClient.query('SELECT * FROM urls WHERE id=' + id, makeLog(cb));
    };
}

for (i = 1001; i <= 20000; i++) { // build the list of tasks to be done in parallel
    queries.push(makeQuery(i, client));
}

Otherwise, you can as well get rid of the factories altogether:

for (i = 1001; i <= 20000; i++) { // build the list of tasks to be done in parallel
    queries.push(function (cb) { // doQuery
        client.query('SELECT * FROM urls WHERE id=' + i,
            function (e, results, fields) { // logResults
                results = results[0];
                results.key = radix64.fromNumber(results.id); // yep, I'm converting the
                                                              // number into base-64 notation
                cb(); // if I wanted to do something useful at the end, I would have
                      // called cb(results) instead, which compiles an array of results
                      // to be accessed by the final callback
            });
    });
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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