1
\$\begingroup\$

What should I consider in order to refactor the following code ?

It seems very close to proudly become a Pyramid of Doom:

router.get('/:id', function(req, res) {

    getId = req.params.id
    obj = {}

    db.con.query('SELECT * FROM employees where id=?', getId, function(err, results) {

        if (err) {
            console.log('error in query')
            return

        } else {
            try {
                obj = {
                    id: results[0].id,
                    name: results[0].name,
                    location: results[0].location
                }
                res.render('showuser')

            } catch (err) {
                res.status(500)
                res.render('error', { error: err, id: getId })
            }
        }
    })
});
\$\endgroup\$
  • \$\begingroup\$ Really doesn't look that bad, regardless consider using async / await for more succinct code. Presumably the DB lib you're using supports Promises. \$\endgroup\$ – James Dec 9 '17 at 0:51
1
\$\begingroup\$

Are we actually using the obj variable? It seems like you are not using it unless you were intending to pass to the render function. I don't think the try block is necessary. You can first check if results contains elements in it, so doing results[0].X will not return you an error since results[0] will never be undefined.

Here's an improved version:

router.get('/:id', function(req, res) {
  const getId = req.params.id;

  db.con.query('SELECT * FROM employees where id = ?', getId, function(err, results) {
    if (err || !results.length) {
      console.log('error in query');
      res.status(500).render('error', { error: err, id: getId });
      return;
    }

    var obj = {
      id: results[0].id,
      name: results[0].name,
      location: results[0].location
    };
    res.render('showuser', obj); // Assuming you're using obj to render
  });
});
\$\endgroup\$
0
\$\begingroup\$
router.get('/:id', function(req, res) {

    const getId = req.params.id

    db.con.query('SELECT * FROM employees where id=?', getId, function(err, results) {

        if (err) {
            console.log('error in query')
            return

        }

        try {
            const obj = {
                id: results[0].id,
                name: results[0].name,
                location: results[0].location
            }
            res.render('showuser')

        } catch (err) {
            res.status(500)
            res.render('error', { error: err, id: getId })
        }
    })
});

There are a few of things I changed

  • added const to getId now it may not interfere with other variables called getId in your module
  • moved definition of the obj inside the try block, since this is the only place you are using it as well as added const for the same reason
  • since you have return in your if (err) condition, the function will end there if it errors. Therefore, else is unnecessary here and was removed
\$\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.