Updating an incoming PUT query

Question

I have the following spaghetti update method and I would like to improve its coding style.

This takes an incoming PUT query for a survey object and updates it accordingly.

update: function(req, res) {
 var slug = req.body.slug
 , errors = {}
 , surveyDiff = req.body.diff;
 surveyDiff.slug = slug;
 // Certains fields are not supposed to be updated, therefore they need to be removed before performing the update.
 var forbiddenFields = [ 'telerivetId', 'telerivetProjectId',
 'votoId', 'votoApiKey', 'campaign', 'country' ];
 forbiddenFields.forEach(function(forbiddenField) {
 if (surveyDiff.hasOwnProperty(forbiddenField))
 delete surveyDiff[forbiddenField];
 });
 if (Object.keys(surveyDiff).length > 1)
 // We need to verify that the user is logged and is the author of the current survey.
 Session.getUser(req.cookies.logged.token, function(err, user) {
 var updateSurvey = function(surveyDiff) {
 // Once all verifications are done, we can safely update the survey.
 Survey.update({ slug: slug, user: user.phoneNumber }, surveyDiff)
 .exec(function(err, surveys) {
 if (!err)
 res.json({ success: true, survey: surveys[0] });
 else
 res.json({ success: false });
 });
 };
 // The order property correspond to a parent model called SurveyGroup, therefore this one is updated separately
 if (!surveyDiff.hasOwnProperty('order'))
 updateSurvey(surveyDiff);
 else
 Survey.findOne({ slug: slug }).exec(function(err, survey) {
 SurveyGroup.update({ id: survey.surveyGroup }, { order: surveyDiff.order },
 function(err, surveyGroup) {
 console.log('update survey group', err, surveyGroup);
 delete surveyDiff.position;
 if (!err) updateSurvey(surveyDiff);
 else res.json({ success: false });
 });
 });
 });
 else res.json({ success: false });
}

This code is running on node.js using sails.js and waterline.js.

Answer 1

For the first part of your code, it could be refactored into the following for readability and "better" practice:

update: function(req, res) {
 // I usually recommend one var per variable to explicitly say "this is a variable"
 // as well as not fumble with commas. Commas have a lot of uses in JS. I'd
 // like to avoid confusion.
 // Arrays can be indented this way to make it look like a list. Additionally,
 // this is a "constant", so let's make it appear like so. And let's pull it up
 // the scope, away from the action, because it does no action.
 var FORBIDDEN_FIELDS = [
 'telerivetId',
 'telerivetProjectId',
 'votoId',
 'votoApiKey',
 'campaign',
 'country'
 ];
 var slug = req.body.slug;
 // Highly suggest you not modify objects you don't own, especially ones
 // generated by the system. That's because something else might be need them.
 //
 // Instead of stripping, why not create one that doesn't include forbidden
 var surveyDiff = Object.keys(req.body.diff).reduce(function(carry, key){
 if(!~FORBIDDEN_FIELDS.indexOf(key)) carry[key] = surveyDiff[key];
 return carry;
 }, {});
 // We can coerce a value to a boolean, and get rid of "> 1"
 // As a colleague would say "What is 1? Is that arbitrary? What does it mean?"
 var hasDiff = !!Object.keys(surveyDiff).length;
 return hasDiff ? update(slug, diff) : res.json({ success: false });
}

As for the second part, callbacks look fine. Though there is a way to prevent "callback hell" and that is to break away the callbacks as named functions, and let the operation refer to them by name. Something like

// Something like this:
function asyncRoutine(){
 db.findSomething({...}, function(err, data){
 db.findAnotherSomething({...}, function(err, data2){
 // data
 });
 });
}
// into something like this:
function asyncRoutine(){
 db.findSomething({...}, handleDbFind);
}
function handleDbFind(err, data){
 db.findAnotherSomething({...}, handleAnotherDbFind);
}
function handleAnotherDbFind(err, data){
 // done!
}

However, you lose the advantage of scopes (visibility of outer variables in nested callbacks). It also makes the code rather long, since you're splitting them up.