# Updating an incoming PUT query

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.
// 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'))

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;

else res.json({ success: false });
});
});
});

else res.json({ success: false });
}


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

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.