1
\$\begingroup\$

I wrote this queue object to handle async functions in JavaScript. I'm not sure if this is a correct approach. Is this steady enough to run on nodeJS or it may fail/fault?

var queue = {
    callbacks: [],
    start: function(){
        this.returnVal = this.callbacks.shift()();
    },
    next: function(returnVal){
        try{
            queue.returnVal = queue.callbacks.shift()( returnVal );
        } catch(e) { console.error("oops, this function failed..:", e)}
    },
    add: function( newCallback ){ // adds new funcion to the end of the queue
        this.callbacks.push(newCallback);
    }
}

/* generic functions */

function f1(param1){
    console.log('running f1');
    console.log('param1 is:', param1);
    queue.next('this string was returned from f1'); // return
}
function f2(param1){
    console.log('running f2');
    console.log('param1 is:', param1);
    setTimeout(function(){ // simulate async
        console.log('adding f9 function to the queue');
        queue.add(f9);
        queue.next('this string was returned from f2 after adding f9 to the queue'); // f2 return
    }, 1000);
}
function f3(param1){
    console.log('running f3');
    console.log('param1 is:', param1);
    queue.next(); // f3 return
}

function f9(){
    console.log('running f9');
    queue.next('f9 returns');
}

queue.callbacks.push(f1, f2, f3);
queue.start();

the output is:

running f1
param1 is: undefined
running f2
param1 is: this string was returned from f1
adding f9 function to the queue
running f3
param1 is: this string was returned from f2 after adding f9 to the queue
running f9
oops, this function failed..: [TypeError: queue.callbacks.pop(...) is not a function]

Note that the last line error is due to queue is now empty. Can the try catch there save nodeJS from crashing?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

One thing I find problematic is that queue.next is visible and can openly be called. The problem arises if your async function is waiting, and something else suddenly shifts the queue. You would get an unexpected results. To remedy this, I would suggest hiding next so that only the callbacks are aware of it.

One way to do it is pass in next as an argument to the callback. next would be a function that simply calls the next callback. If I remember correctly, this is how express did middleware.

function callback(arg1, arg2, ..., next){
  // do stuff
  next(arg1, arg2, ...);
}

An alternative would be to return a Promise. An advantage with this approach is that you remove the complexity of having to wrap the next callback as the next argument and instead, just call the callback and expect a Promise.

function callback(resolvedValue){
  // do stuff
  return Promise.resolve(resolution);
}

For your error, you should not rely on try-catch. try-catch should only be used for unexpected errors or errors that are intentionally thrown (like from an API) and meant to be recovered. But in this case, you are aware that it will throw an error when the queue is empty. You should do the check if the queue is empty before proceeding instead of relying on try-catch.

You should consider hiding the queue array. The problem with it being in the open is that it can be modified freely. It might be altered unintentionally, causing odd results. Consider hiding it inside a closure.

Your queue should hold a flag indicating that it's running or paused. Your queue could be in the middle of an async operation when queue.start is called. You wouldn't want the queue to suddenly start running. Also, if your queue runs out of callbacks, what does it do? Does it pause? Does it pick up after receiving a callback? How does third-party code know if the queue is still processing? These are some things to consider, and it depends on how the queue is used.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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