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.