Controller method to validate and save

Question

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);
    }
  });
}

Answer 1

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);
  });

}

Answer 2

Readable code is declarative and concise.

While it's true in the general case that too much abstraction can sometimes damage readability, it can sometimes improve it too. In your example, the refactored code is not only shorter, but also expresses its intent more declaratively.

When you say "now the code doesn't express the flow," I think you are confusing the ability to step through the code in your mind as an interpreter would with readability. You shouldn't have to, or even want to, understand code by stepping through it in your mind: good code tells you in English what it does -- the rest is implementation details that are irrelevant unless you're debugging.

Your argument is a bit like saying that:

arr = [1, 2, 3];
squared_arr = [];
for (var i=0; i<arr.length; i++) {
    squared_arr[i] = arr[i]*arr[i];
}

is more readable than:

arr = [1, 2, 3];
squared_arr = arr.map(function(x) { return x*x });