Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Is this approach considered good practice, or should I create separate router.delete functions for all my routes? Please, explain why.

router.delete('/delete-:object', function(req, res) {
   var query;
   var id = req.body.id;

   switch (req.params.object) {

       case 'news' :
           query = queries['news_delete'];
           break;

       case 'member' :
           query = queries['member_delete'];
           break;

       case 'account' :
           query = queries['account_delete'];
           break;

       default :
           res.sendStatus(404);
           return;
   }

   connection.query(query, id);
   res.sendStatus(200);
});
share|improve this question

If most of your code is the same among all the different delete URLs you are processing, then it makes perfect sense to share one route that branches based on the minor difference between them. Since that is what it looks like you have, then the way you have it is a logical way to do it.

You could simplify your current technique a bit:

router.delete('/delete-:object', function(req, res) {
   var query, item = req.params.object;

   switch (item) {
       case 'news' :
       case 'member' :
       case 'account' :
           query = queries[item + '_delete'];
           break;
       default :
           res.sendStatus(404);
           return;
   }

   connection.query(query, req.body.id);
   res.sendStatus(200);
});

Or, you could even remove the switch statement in favor of a lookup table:

router.delete('/delete-:object', function(req, res) {
   var query, item = req.params.object;
   var deleteObjects = {news: true, member: true, account: true};

   if (deleteObjects.hasOwnProperty(item)) {
       connection.query(queries[item + '_delete'], req.body.id);
       res.sendStatus(200);
   } else {
       res.sendStatus(404);
   }
});

You could also move the common code to a function with an argument and do it this way:

function processDelete(q) {
    return function(req, res) {
        connection.query(queries[q], req.body.id);
        res.sendStatus(200);        
    }
}

router.delete('/delete-news', processDelete("news-delete"));
router.delete('/delete-member', processDelete("member-delete"));
router.delete('/delete-account', processDelete("account-delete"));

I'd say that there is no "right" or "wrong" argument about which of these approaches is correct. The last approach is perhaps a little quicker to read exactly what it's doing since the routes are all laid out there for you and the handler function is all right there and there's no mixing of the logic of the two, but that's as much a personal opinion as anything else, not really a firm argument one way or the other.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.