3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

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.