3

I am in the process of relearning Javascript and last week when writing this code for a university assignment I think that there is probably a much better way of executing this code

app.get('/member/all', function(req, res) {    
    connection.query('CALL GetAllMembers()', function(err,rows){
        connection.query('CALL CountMembers()', function(err, allMembers){
            console.log(err);
            connection.query('CALL CountAllIndMembers()', function(err,indMembers){
                console.log(err);
                connection.query('CALL CountInactiveMembers()', function(err,inactiveMembers){
                    console.log(err);
                    connection.query('CALL CountAllMembersInGroups()', function(err,groupMembers){
                        console.log(err);
                        res.render('members', {members : rows[0], title : "All Members",  groupMembers : groupMembers[0][0].AllGrpMembers,
                            inactiveMembers : inactiveMembers[0][0].AllInactiveMembers, indMembers : indMembers[0][0].AllIndMembers,
                            allMembers : allMembers[0][0].AllMembers, statistics : true});
                        });
                    });
                });
            });
        });
    });
});

When I was trying to declare variables under the app.get such as var allMembers... when the callback was executed I was unable to set allMembers = rowsFromTheCallback. It seemed that it was a local variable to that callback. I'm sure this is something to do with the variable scope and/or hoisting. Just wanted to ask you guys if there would be a better way to do this as even though this function works. It is very ugly to look at haha!

Thanks in advance

Jack

Rodrigo Medeiros
  • 7,814
  • 4
  • 43
  • 54
  • Make them all functions rather than calling them all anonymously! – Callum Linington May 11 '14 at 15:08
  • The uglyness of the code is an unfortunate sideeffect of node callbacks. I would recommend taking a look at something involving generators (yield keyword instead of callbacks) but don't know if stable enough to use in Node yet. If you want something more traditional you can look at libs like async.js or something using Promises – hugomg May 11 '14 at 15:25
  • Reading about "Contination Passing Style" will add greatly to you understanding at this point. – Jason Aller May 11 '14 at 16:39

3 Answers3

1

As far as scope goes, all the inner functions should be able to read and write to the outer variable unless it is shadowed by an inner variable declaration or function parameter.

The problem you are having might be related to the async-ness of the code. See this code:

function delay(n, cb){
    setTimeout(function(){ bs(delay) }, delay);
}

function main(){
    var allMembers = 17;
    delay(500, function(){
        console.log(allMembers);  // This looks at the outer "allMembers"
        allMembers = 18;

        delay(200, function(allMembers){  // <-- SHADOW
            console.log(allMembers); // This looks at the allMembers from "delay 200"'s callback
            allMembers = 42;
        });

        delay(300, function(){
            console.log(allMembers); //This is the outside "allMembers" again
        });
    });

    return allMembers; // Still 17!
}

main();

main will return before the setTimeouts have even fired so its going to return the original value of that variable. In order to wait for the inner callbacks to run, the only way is to make main take a callback to signa when its done, instead of just returning.

function main(onResult){
   delay(500, function(){
      //...
      onResult(allMembers);
   });

   // <-- no return value
});

main(function(allM){
    console.log(allM);
}); 
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
hugomg
  • 68,213
  • 24
  • 160
  • 246
  • thanks that has cleared it up for me :) – user3621357 May 11 '14 at 15:21
  • 1
    When faced with the need to make a series of calls to a database where each subsequent call depends on the return from the previous call this method will result in less than optimal performance. Worse yet, if the delays that have been picked are not long enough it will fail. Increase the delays and you only make the performance worse. – Jason Aller May 11 '14 at 16:09
  • @JasonAller: I intended the delay functions just as some simple async function that you can use to test stuff without a DB and demonstrate the variable scoping. Yes, I wouldn't recommend using actual delays to wait for DB responses (in fact, nested DB calls are often a smell - its always worth investigating if there isn't a way to get the same data with jsut a single query) – hugomg May 11 '14 at 18:09
  • My apologies, I missed some of your answer on my first read through. I was concentrating on a different aspect of the question when I read through your answer. – Jason Aller May 11 '14 at 21:52
1

See async library: https://github.com/caolan/async

async.series([
  getAllMembers,
  countMembers,
  ...
], function(err, results) {   
  // err contains an error if any of the functions fails. No more functions will be run.
  // results is an array containing results of each function if all the functions executed without errors
}));

function getAllMembers(callback) {
  connection.query('CALL CountMembers()', callback);
}

function countMembers(callback) {
 ...
}

If the execution order of the functions does not matter, async.parallel can be used instead of async.series.

Matti Lehtinen
  • 1,694
  • 2
  • 15
  • 22
0

There is power in using a library to handle and encapsulate "Continuation Passing Style" (CPS) interactions with your asynchronous calls. The following code isn't from a library, but I'm going to walk through it and use it as an example of one way to implement CPS.

Setting up a scope appropriate queue is the first step. This example uses about the most simple method for doing so:

var nextList = [];

After that we need a method to handle our first case, the need to queue tasks to be performed in the future. In this case I was focused on performing them in order so I named it next.

function next() {
    var todo,
        current,
        task,
        args = {};
    if (arguments.length > 0) { // if called with parameters process them
        // if parameters aren't in an array wrap them
        if (!Array.isArray(arguments['0'])) {
            todo = [arguments];
        } else { // we were passed an array
            todo = [];
            arguments['0'].forEach(function (item) {
                // for each item we were passed add it to todo
                todo.push(item);
            });
        }
        nextList = todo.concat(nextList);
        // append the new items to the end of our list
    }
    if (nextList.length > 0) { // if there are still things to do
        current = Array.prototype.slice.apply(nextList.shift());
        task = current[0];
        args = current.slice(1);
        task.apply(null, args); // execute the next item in the list
    }
}

This allows us to make calls like:

.map(function (filepath) {
    tasks.push(
        [
            handleAsset,
            {
                'path': filepath,
            }
        ]
    );
});
tasks.push([done]);
next(tasks);

This will call handleAsset, which is async, once for each file, in order. This will allows you to take your code and change each of the nested calls into a separate function in the form:

function memberAll() {
    app.get('/member/all', function(req, res) {
        if (err) {
            handleError(err, 'memberAll');
        } else {
            next(getAllMembers, 'parameters to that call if needed');
        }
    });
}

where handleError is a common error handler, and the next call allows you to pass on relevant parameters to the next function that is needed. Importantly in the success side of the if statement you could either:

  • conditionally call one of several functions
  • call next with an array of calls to make, for instance if you had functions for processFolder and processFile you could expect that processing a folder might involve processing other folders and files and that the number would vary
  • do nothing except call next() with no parameters and end the current branch

Embellishments can include writing a clean function for emptying the nextList, adding items to nextList without calling an item from the list, etc. The alternative at this point is to either use an existing library for this or to continue writing your own.

Jason Aller
  • 3,541
  • 28
  • 38
  • 38