Controller method to validate and save I have built RESTful web services. The services endpoints have a similar logic:


If the input validation succeeds, do a database operation and return the result.
If the db operation fails, return the db error.
If the validation fails, return a response with information on the failed validation.


My code was explicit in the sense you can read the code and understand the flow of the program. A coworker refactored it so that this basic flow is refactored in a utility class which main function takes the request body, the db operation and the handlers. Now the code doesn't express the flow but the basic logic is refactored.

Which code better follows best practice? Would you sacrifice readability for refactoring? Yes, the unit test will be reduced a bit too.

Example:

Before:

save: function (req, res, next) {
 var dao = topicsDaoFactory.create(req.dbSession);

 validate(req.body.topic, constraintsObj,
   function success () {
     dao.save(req.body.topic, function saveHandler (err, result) {
       if (err) {
         res.error(err);
       } else {
         res.send(result);
       }
     });
   },
   function error (errors) {
     res.fail(errors);
   });
}


After:

save: function (req, res) {
  var dao = topicsDaoFactory.create(req.dbSession);

  ControllerUtils.validateAndTryOperation(req, res, {
    input: req.body.topic,
    constraints: constraintsObj,
    operation: function (input, callback) {
      dao.create(Topic.createFromScratch(input), callback);
    }
  });
}
What is the best practice? Would you sacrifice readability for refactoring?


That highly depends on the case. Most of the time I write code for readability. Most of my code reviews here focus more on readability. Only if there's sufficient proof that my code is a bottleneck do I actually refactor it.

So, yes, I prefer your old code. However, it could be done a bit better if you used Promises. It's longer, but avoids callback pyramid. There's a package called BlueBird that converts callback-style APIs into Promises.

save: function (req, res, next) {
  var dao = topicsDaoFactory.create(req.dbSession);

  // Try validating
  validate(req.body.topic, constrainsObj)
  // Listen for validate
  .then(function(){
    return dao.save(req.body.topic);
  }, function(error){
    res.error(error);
  })
  // Listen for dao.save
  .then(function(result){
    res.send(result);
  },function(error){
    res.fail(error);
  });

}