CODEWITHSUNDEEP
javascriptnode.jsexpress
Engineering Machine
Engineering Machine

Reputation: 642

Node.js parameterized route - good or bad practise?

This is my code:

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

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

Upvotes: 1

Views: 97

Answers (3)

Mut1aq
Mut1aq

Reputation: 11

For anyone reading this, this is bad practice, it directly violates the Open-Closed Principle. The open–closed principle (OCP) states "software entities (classes, modules, functions, etc.) should be open for extension, but closed for modification". This would mean that if you write code that needs to be modified each time you add something new, it violates the Open-Closed Principle. The reason most new developers fall into this is because they think their code should always follow the DRY principle which is actually not applicable in these cases.

Upvotes: 1

Eric Owle
Eric Owle

Reputation: 21

From a technical standpoint - I don't see anything wrong with this approach.

However, do you need to even have the 'delete-' prefix before the object? You already know a delete is being issued via HTTP, so its fairly repetitive.

This is a matter of opinion, but I would use:

router.delete('/:object', function(req, res) {
  var queryName = req.params.object + '_delete';

  if(queries.hasOwnProperty(queryName) === false) {
    res.sendStatus(404);
    return;
  }

  connection.query(queries[queryName], req.body.id);
  res.sendStatus(200);
});

Alternatively, you could refactor your queries object so that it has a delete field that is an object of your delete queries, and avoid munging the name altogether:

var queries = {
  delete: {
    'news': '...',
    'member': '...',
    'account': '...'
  }
};

Now your object for your delete route will map 1:1 to your queries.delete object.

router.delete('/:object', function(req, res) {
  if(queries.hasOwnProperty(req.params.object) === false) {
    res.sendStatus(404);
    return;
  }

  connection.query(queries[req.params.object], req.body.id);
  res.sendStatus(200);
});

The other advantage here is that if you add a new object type, you won't need to continually update the switch, as in your original question.

Upvotes: 2

Romeo Kienzler
Romeo Kienzler

Reputation: 3537

I'd say good practise, because it is very generic and you always should avoid repetitions in stupid code since you'll likely make mistakes

you can even generalize a bit more

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

connection.query(queries[req.params.object], id);
res.sendStatus(200);
});

(assuming you have named the queries the same way as the objects

I additionally assume you will secure the route so that nobody without login can delete objects.

Upvotes: 0

Related Questions