Today’s lesson: Delegate the hard work orthogonal to your business logic – concurrency, asynchronicity, state maintenance during iteration etc. – to your language and API if you can.
They are more likely to get it right and your code will be more focused on what you are actually trying to do. You should feel an unpleasant tingling when you have do these manually.
We will look at JavaScript code that uses asynchronous calls to get data in a too manual (and incorrect) way and improve it to delegate the necessary synchronisation to the promises library Q that it already uses.
The code presented below does this:
- Asynchronously fetch users from MongoDB
- For each user, asynchronously fetch her stats from Fitbit (skip those that fail to fetch)
- Return a promise of an array with users including their stats
Since the code does a lot and might be thus difficult to read, here is a simplified version:
// ##### ORIGINAL (simplified)
var deferredResults = Q.defer();
mongo.findAllAsync(function(users) {
var userStats = [];
users.forEach(function(user) {
// var nrCompleted = 0;
fetchUserStatsAsync(function(stats) {
userStats.push({user: user, stats: stats});
// should be: if (++nrCompleted === users.length)
if (users.indexOf(user) === users.length-1) {
deferredResults.resolve(userStats);
}
});
});
});
return deferredResults.promise;
// ##### IMPROVED (simplified)
var deferredResults = Q.defer();
mongo.findAllAsync(function(users) {
var userStatsPromises = users.map(function(user) {
var userStatsPromise = Q.defer();
fetchUserStatsAsync(function(stats) {
userStatsPromise.resolve({user: user, stats: stats});
});
return userStatsPromise.promise;
});
return deferredResults.resolve(Q.all(userStatsPromises));
});
return deferredResults.promise;Here is the original code:
var Q = require('q');
var mongoFacade = require('mongoFacade');
/** For each user in our DB, fetch her stats from Fitbit */
var getUsersWithFitbitStats = function(){
var deferred = Q.defer();
//get stats for all users:
var userStats = [];
mongoFacade.find({}, function(err, users){ // Async fetch from DB
users.forEach(function(user){
Auth.oauth.getProtectedResource(
'https://api.fitbit.com/1/user/-/activities.json',
'GET',
user.oauthAccessToken,
user.oauthAccessTokenSecret,
function (error, data, response) { // Async fetch stats from Fitbit
if (error) {
logError(error, user);
return;
}
var responseData = JSON.parse(data);
var steps = responseData.lifetime.total.steps;
userStats.push({
user: {
id: user.fitbitId,
name: user.name
},
data: responseData
});
//complete promise if this is the last element in array:
if (users.indexOf(user) === users.length-1) {
return deferred.resolve(userStats);
}
}
);
});
});
return deferred.promise;
};
module.exports = {
getUsersWithFitbitStats: getUsersWithFitbitStats
}; The main problem is that we try to manually detect that all partial requests for user stats have been completed and then resolve the promise that the function returns. It also contains bugs, such as relying on the last promise in the array to be resolved last (instead of f.ex. counting the number of promises completed so far and resolving when all users.length promises are done).
Here is an improved version that uses Q’s allSettled to implement this waiting for the partial promises. It is also nicer in the respect that it does not swallow individual failures but returns them up the stack (where they are later explicitly filtered away).
Highlights:
- Line 10 – use map to transform users into promises of user+stats
- Line 23 – explicitely reject users whose stats failed to fetch
- Lines 30, 40 – return a promise of user+stats
- Line 47 – wait for all the promises to be completed, filter out failures, keep values
var Q = require('q');
var mongoFacade = require('mongoFacade');
/** For each user in our DB, fetch her stats from Fitbit */
var getUsersWithFitbitStats = function(){
var deferredResults = Q.defer();
mongoFacade.find({}, function(err, users){ // Async fetch from DB
// Turn each user into a promise of user stats
var userStatsPromisses = users.map(function(user){
var userStatsPromise = Q.defer();
Auth.oauth.getProtectedResource(
'https://api.fitbit.com/1/user/-/activities.json',
'GET',
user.oauthAccessToken,
user.oauthAccessTokenSecret,
function (error, data, response) { // Async fetch stats from Fitbit
if (error) {
logError(error, user);
userStatsPromise.reject(error); // don't hide the failure from the caller
return;
}
var responseData = JSON.parse(data);
// Deliver the promissed data
userStatsPromise.resolve({
user: {
id: user.fitbitId,
name: user.name
},
data: responseData
});
}
);
return userStatsPromise.promise; // return an element of the new array
}); // users.map
// Wait for all of the userStats promisses to finish, filter out failures,
// return the rest as an array of user stats
//userStatsPromisses; -> deferredResults
Q.allSettled(userStatsPromisses).then(function (results) {
var fulfilled = function(r) {
return r.state === "fulfilled";
};
var getValue = function(r) {
return r.value;
};
var goodUserStats = results.filter(fulfilled).map(getValue);
deferredResults.resolve(goodUserStats);
});
}); // async mongoFacade.find
return deferredResults.promise;
};
module.exports = {
getUsersWithFitbitStats: getUsersWithFitbitStats
}; (Of course the code can certainly be improved, I am no JS master.)
Conclusion
Use languages and libraries that take care of the hard bits of coordination and state management and focus on the business logic. You are less likely to write buggy code.
Related
The post Unconditional Programming by M. Feathers also promotes delegating the hard work of controlling execution to the language (and provides a nice example):
Over and over again, I find that better code has fewer if-statements, fewer switches, and fewer loops. Often this happens because developers are using languages with better abstractions. [..] The problem with control structures is that they often make it easy to modify code in bad ways.
| Reference: | Let the API Do the Hard Work: Promises in JavaScript with Q from our WCG partner Jakub Holy at the Wonders of Code blog. |
