2
\$\begingroup\$

Steps:

  • Get AMOUNT from a call to API endpoint /a, e.g. AMOUNT == 5 (in the code it is set to 2)
  • Use this AMOUNT to make AMOUNT calls to endpoint /b receiving JSON
    • Result might be: [{a: 1, b: 2}, {...}, {...}, {...}, {...}]
  • Use this AMOUNT to make AMOUNT calls to endpoint /c receiving JSON
    • Result might be: [{c: 3, d: 4}, {...}, {...}, {...}, {...}]
  • Merge such that there is a resulting array of JSON objects
    • Result might be: [{a:1, b:2, c: 3, d: 4}, {...}, {...}, {...}, {...}]

So pretty much it looks like this:

          a  
         / \
        b   c
         \ /
        result

I'm really trying to understand how to write nice NodeJS code (coming from Python), so I'm specifically looking for the most elegant way to do this. I hope people can give advice on my current code below, as well as perhaps just suggesting different though clearly better methods. E.g. doing everything in async but still merging into the same result.

I'm also interested in which of the 3 styles of options (1/2/3) is best for writing NodeJS apps.

Promise = require("bluebird");
u = require("underscore")

function get_phase_count() {
    return new Promise(function(resolve, reject) {
        resolve(2); // AMOUNT == 2
    });
}

function get_phase_info(amount) {
    console.log(amount) // this would be used but now it is mocked
    return new Promise(function(resolve, reject) {
        // note that I got this AMOUNT calls solved to deliver this
        resolve([ { a: 1, b: 2 }, { a: 1, b: 2 } ]);
    });
}

function get_phase_votes(amount) {
    console.log(amount) // this would be used but now it is mocked
    return new Promise(function(resolve, reject) {
        // note that I got this AMOUNT calls solved to deliver this
        resolve([ { c: 3, d: 4 }, { c: 3, d: 4 } ]);
    });
}

function join_phase_promises(p1, p2) {
    return Promise.all([p1, p2]);
};

Using those functions I wrote 3 style options:

// Option 1
get_phase_count().then(function(amount) {
    join_phase_promises(get_phase_info(amount), get_phase_votes(amount)).then(function(args) {
        console.log(u.map(u.zip(args[0], args[1]), function(x) {return u.extend(x[0], x[1])}));
    })
});

// Option 2
get_phase_count()
.then(function(amount) {
    return join_phase_promises(get_phase_info(amount), get_phase_votes(amount));
})
.then(function(args) {
    console.log(u.map(u.zip(args[0], args[1]), function(x) {return u.extend(x[0], x[1])}));
})

// Option 3
get_phase_count().then(function(amount) {
    return join_phase_promises(get_phase_info(amount), get_phase_votes(amount));
}).then(function(args) {
    console.log(u.map(u.zip(args[0], args[1]), function(x) {return u.extend(x[0], x[1])}));
})
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Nice job on your first question. \$\endgroup\$ – Phrancis Mar 5 '16 at 21:17
2
\$\begingroup\$

Option 1 defeats the whole purpose of Promises. You are nesting the callbacks. Option 2 and 3 are not different, just how they're formatted. That's up to personal preferences.

Now going with the fact that get_phase_count, get_phase_info and get_phase_votes are wrapper functions to API calls, let's not touch those. That leaves us with join_phase_promises. join_phase_promises is unnecessary as it's just a wrapper for Promise.all which you can simply use directly.

Now over to code styles, JS follows camelCase convention. This makes your function names, which are underscore-cased, look like outcasts. Suggesting you use camelCase instead.

If you happen to use Node.js 4+ (current version is 5.7), the native Promise is already available. No need to use Bluebird for that. You can also start using arrow functions to make your callbacks a bit more compact. Lastly, JS has a bunch of array operations and recently included destructuring. There's little need to use underscore.

If I were to write your function, it would go like

getPhaseCount().then(amount => Promise.all([
  getPhaseInfo(amount),
  getPhaseVotes(amount)
])).then(args => {
  let merged = args.reduce((p,c) => c.map((a, i) => ({...a, ...p[i]}) ), []);
  console.log(merged);
});

The above logic took me a while to figure out using only native operations, but what it does is to allow us to see the previous and current values using reduce. This way, on each item, map essentially creates a new array containing a merged version of the previous and current.

\$\endgroup\$
  • \$\begingroup\$ Thanks for all the feedback, it's really useful! I'll use it all. I just do not understand this part: {...a, ...p[i]}, somehow does not seem symmetric? The ... are not real right? \$\endgroup\$ – PascalVKooten Mar 6 '16 at 9:31
  • \$\begingroup\$ To add: everything worked when just substituting the let merged with the "underscore way", but of course I'm still interested in getting it to work like this. \$\endgroup\$ – PascalVKooten Mar 6 '16 at 9:55
  • \$\begingroup\$ @Dualinity It's actually real. I used to use ... to mean "everything else", but now it's actually part of JS just recently. \$\endgroup\$ – Joseph Mar 6 '16 at 15:40

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.