Forbid function calls in multiple lines
Closed this issue · 9 comments
// Bad
const responseBody = singleErrorResponse(
req,
'action_forbidden',
null,
`Transação com status '${transaction.status}' não pode ser estornada`
)
// Good
const type = 'action_forbidden'
const param = null
const message = `Transação com status '${transaction.status}' não pode ser estornada`
const responseBody = singleErrorResponse(req, type, param, message)
// Good
const responseBody = singleErrorResponse({
req,
type: 'action_forbidden',
parameter: null,
message: `Transação com status '${transaction.status}' não pode ser estornada`
})
// Good
const even = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10].filter(number => {
return number % 2 === 0
})I don't agree with not allowing to split function arguments.
This is necessary when you integrate with third party APIs that you don't have control to.
@derekstavis For those situations, you can do as in the example:
// Good
const type = 'action_forbidden'
const param = null
const message = `Transação com status '${transaction.status}' não pode ser estornada`
const responseBody = singleErrorResponse(req, type, param, message)@thalesmello I also don't agree with this.
Have you seen our core routes definition? Each route has multiple middlewares that are passed as parameters. Breaking them into lines brings better readability. Also, your example above can't be reproduced there as middlewares are already constants.
@thalesmello I can't post here as it is from a private repo, but... something similar to this:
app.get(
'/users',
middlewareA,
middlewareB,
middlewareC,
middlewareD,
usersController
)vs
app.get('/users', middlewareA, middlewareB, middlewareC, middlewareD, usersController)PS: middlewares names are much longer as they have semantic meaning.
@mccraveiro You could do:
const loggedUser = [
middlewareA,
middlewareB,
middlewareC,
middlewareD,
]
app.get('/users', ...loggedUser, usersController)Or even better, we could use the adequate express abstraction for this.
const loggedUser = express.Router()
loggedUser.use(middlewareA)
loggedUser.use(middlewareB)
loggedUser.use(middlewareC)
loggedUser.use(middlewareD)
app.get('/users', loggedUser, usersController)Or even even better
const handleUser = express.Router()
handleUser.use(middlewareA)
handleUser.use(middlewareB)
handleUser.use(middlewareC)
handleUser.use(middlewareD)
handleUser.use(usersController)
app.get('/users', handleUser)Repeating the same point I said in #10, splitting function call across
multiple lines leads to bad design. It's much more convenient to break
your function call across multiple lines rather than giving good names
for the abstractions in your systems.
However, I will choose the second option anytime, as it leads to a code that's
easier to read and understand, since variables with good names are documentation.
Another plus side, we would also get a consistent dangling comma behavior. Special cases aren't special enough to break the rules.
@derekstavis @MarcoWorms @grvcoelho May I get your comments?
Or even better
const handleUser = express.Router()
[
middlewareA,
middlewareB,
middlewareC,
middlewareD,
usersController,
]
.forEach(handleUser.use)
app.get('/users', handleUser)I continue believing that splitting arguments in parenthesis is a good pattern.
I will be closing this issue since we've merged #19.