2
\$\begingroup\$

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);
    }
  });
}
\$\endgroup\$
  • \$\begingroup\$ What is the unused next parameter for? \$\endgroup\$ – 200_success Sep 22 '15 at 5:51
  • \$\begingroup\$ It is an optional parameter to pass the command to the next middleware. For example, if you have an error, you could call next(error) and have a middleware than handle the error (display an error page and what not). \$\endgroup\$ – ontk Sep 23 '15 at 17:58
3
\$\begingroup\$

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

}
\$\endgroup\$
3
\$\begingroup\$

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 });
\$\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.