Is there a better way to handle errors in express?

user_14910549 0
Viewed 39

I'm building an API in express after some time away from it. This app helps users track their guitar routines. Here is an example of a DELETE function in my controller and the possible exceptions that can be thrown:

 deleteOneRoutine = async (userId, routineId) => {
  // If the given routineId is found in the DB 
  // AND the userId matches, delete it from DB

  const selectedRoutine = await Routine.findOne({_id: routineId});
  if (!selectedRoutine) {return 400};

  if (selectedRoutine.userId != userId) {return 401};

  const deleted = await selectedRoutine.remove();
  if (!deleted) {return 500};

  return deleted;
}

And here is the route that receives those error codes:

  routineRouter.delete('/:routineId', async (req, res) => {
   const userId = req.params.userId;
   const routineId = req.params.routineId;
   const deleted = await routineController.deleteOneRoutine(userId, routineId);
   if (deleted === 400) {res.status(400).send('Requested routine could not be found.')}
   else if (deleted === 401) {res.status(401).send('Unauthorized user.')}
   else if (deleted === 500) {res.status(500).send('Server error. Could not delete routine.')}
   else {res.status(200).send(`Successfully deleted routine with ID ${routineId}`)}  
});

I'm wondering if there is a better way to handle this instead of manually passing back the error codes from the controller. Any help would be appreciated!

express node.js api-design javascript
2 Answers

I'm not so happy about the current typing of the return value - I think returning an object with either an error in an error property or the routine ID would make more sense. But a better way IMO would be to pass down the res so that deleteOneRoutine can do it itself. Also make a helper function to keep things DRY, and don't forget to catch possible exceptions thrown by the asynchronous functions:

deleteOneRoutine = async (userId, routineId, res) => {
  const send = (code, message) => res.status(code).send(message);
  const serverError = () => send(500, 'Server error. Could not delete routine.');
  try {
    const selectedRoutine = await Routine.findOne({_id: routineId});
    if (!selectedRoutine) return send(400, 'Requested routine could not be found.');
    if (selectedRoutine.userId !== userId) return send(401, 'Unauthorized user.');
    const deleted = await selectedRoutine.remove();
    if (!deleted) return serverError();
    send(200, `Successfully deleted routine with ID ${deleted}`);
  } catch(e) {
    serverError();
  }
}

routineRouter.delete('/:routineId', (req, res) => {
  const { userId, routineId } = req.params;
  routineController.deleteOneRoutine(userId, routineId, res);
});
user_95152070

It would be easier to throw an error and add an error handler in Express.

router.use((err, req, res, next) => {
  res.status(500).send(err.message);
});

router.get('/path', async (req, res) => {
  throw new Error('Can't delete!');
});

I think the error middleware has to come first, but I'd have to check.

If you need specific error types you can subclass Error; if you want to be able to pass a code, you could add a property on to the Error (though I would recommend avoiding mixing routing logic (e.g. setting specific HTTP codes) with business logic (e.g. your deleteOneRoutine function))

user_6757210
Related