2
\$\begingroup\$

I'm currently making an app for my side project and I was looking to get some insight on the main js file, what can I improve, the error handling, what am I doing wrong, etc. I'm mostly confused about the error handling.

This is the main javascript file:

import express from 'express'
import cors from 'cors'
import dotenv from 'dotenv'

dotenv.config()
const morgan = require('morgan')
const app = express()

// Require routes
const usersRouter = require('./routes/users')

// Configs
require('./cfg/mongoose')

// Express middlewares / settings
app.set('port', process.env.PORT || 3000)
app.use(cors())
app.use(express.json())
app.use(express.urlencoded({ extended: false }))

// Environment specific middlewares
if (app.get('env') === 'production') {
  app.use(morgan('combined'))
} else {
  app.use(morgan('dev'))
}

// Routing
app.use('/users', usersRouter)

app.get('/', (req, res) => {
  res.status(200).send({
    message: 'Welcome to the API! 🍺🍸'
  })
})

// Handles 404
app.use((req, res, next) => {
  res.status(404).send({ error: 'Invalid endpoint' })
})

// Generic error handler
app.use((err, req, res, next) => {
  if (err.errors) {
    let errors = {}
    for (const errorName in err.errors) {
      errors = { ...errors, [errorName]: err.errors[errorName].message }
    }
    res.status(400).send({ errors })
  } else {
    res.status(err.output.statusCode).send({
      message: err.message || 'Internal server error'
    })
  }
})

// Server
app.listen(app.get('port'), () => {
  console.log(`Server listening on port ${app.get('port')}`)
})

And this is the users route:

import express from 'express'
import mongoose from 'mongoose'
import User from '../models/User'
import boom from '@hapi/boom'
const router = express.Router()

router.post('/', async (req, res, next) => {
    const { phone_number, email, password, name } = req.body
    try {
        const newUser = await User.create({
            name,
            email,
            phone_number,
            password,
        })
        res.status(201)
            .set('Location', `${req.hostname}${req.baseUrl}/${newUser.id}`)
            .send(newUser.id)
    } catch (err) {
        next(boom.boomify(err, { statusCode: 400 }))
    }
})

router.get('/', async (req, res, next) => {
    let users
    try {
        users = await User.find({}, 'name email').lean()
        Object.values(users).map(user => {
            user.profile_url = `${req.hostname}${req.baseUrl}/${user._id}`
        })
        if (!users.length) next(boom.notFound('Cannot find any users'))
        res.status(200).send(users)
    } catch (err) {
        next(boom.badImplementation())
    }
})

router.get('/:id', async (req, res, next) => {
    try {
        const user = await User.findById(
            req.params.id,
            'name email phone_number'
        )
        res.status(200).send(user)
    } catch (err) {
        next(boom.notFound('User not found'))
    }
})

router.delete('/:id', async (req, res, next) => {
    try {
        await User.deleteOne({ _id: mongoose.Types.ObjectId(req.params.id) })
        res.sendStatus(204)
    } catch (err) {
        next(boom.notFound('User not found'))
    }
})

router.put('/:id', async (req, res, next) => {
    let user
    try {
        const { email, phone_number, password, name } = req.body
        user = await User.findOneById(req.params.id, '-__v')
        user.email = email || user.email
        user.phone_number = phone_number || user.phone_number
        user.password = password || user.password
        user.name = name || user.name
        let updatedUser = await user.save()
        res.status(200).send(updatedUser)
    } catch (err) {
        next(boom.notFound('User not found'))
    }
})

module.exports = router
```
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I wouldn't think that a database error should cause a status of 400 as the route was found just fine. Probably should be 5xx for an internal server error. \$\endgroup\$ Commented Sep 1, 2019 at 16:21
  • \$\begingroup\$ I don't have much comment regarding the error handling, like @jfriend00 said the status of 400 makes more sense if it was 5xx. On another note: I havent used boom but I do like to create a global response object that I can always return in all my requests. I make sure it has a few properties like data, status and mabye error object. This way my front-end that consumes the API will always know exactly what to expect. I know that any data i request, will always be under the data property or any errors will be under my error property. \$\endgroup\$ Commented Apr 13, 2020 at 19:41

0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.