5
\$\begingroup\$

I'm learning and now I'm trying to figure out if something in this code can be improved. This code is expected 3 urls and then async parallel calls should be done. When all requests are finished thet just show each request's data.

const hyperquest = require('hyperquest');
const BufferList = require('bl');
const Promise = require('bluebird');

var tasks = [];
for (var i = 0; i < 3; i++) {
    new function(index) {
        tasks[index] = new Promise(function(resolve) {
            makeRequest(resolve, index);
        });
    }(i);
}

Promise.all(tasks).then(function(result) {
    result.forEach(function(item) {
        console.log(item.bl.toString());
    })
});

function makeRequest(resolve, step) {
    var bl = new BufferList();
    var req = hyperquest(process.argv[step + 2]);
    req.on('end', function() {
        resolve({data: bl});
    });
    req.pipe(bl);
}

The main question is it right usage of Promise. Should I use this

tasks[index] = new Promise(function(resolve) { makeRequest(resolve, index); }); 

or it can be simplified?

Also req.on('end', function() { resolve({data: bl}); }); is a direct call of resolver callback but maybe it's possible without it?

\$\endgroup\$
2
  • \$\begingroup\$ What is the purpose of this code? \$\endgroup\$ Nov 11, 2014 at 20:59
  • \$\begingroup\$ Read 3 urls and then print their content using Promises :) \$\endgroup\$ Nov 11, 2014 at 21:28

1 Answer 1

1
\$\begingroup\$

On the whole it does not look very elegant, especially new function, building functions in a loop and building the Promise array.

I would suggest you invest time in the bluebird Promise.map function and write something like this:

var indexes = [0,1,2];
Promise.map(indexes, function(index) {
    return new Promise(function(resolve) {
        makeRequest(resolve, index);
    });
}).then(function(result) {
    result.forEach(function(item) {
        console.log(item.bl.toString());
    })
});

Other than that, you seem very light on the failure handling, make sure to use catch and reject.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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