refactor: append return statements after response callback #218
Conversation
|
Technically in those cases the function call ends after the response statements. But I think its an improvement to have the explicit return statements to prevent bugs in case anyone adds any lines after. |
|
@JasonChong96 I agree. Additionally, this issue might had been exacerbated by eslint, which right now removes return statements it deems unnecessary. This creates problems as we need to make conscious efforts to remember to add the return statements back when it becomes less unnecessary. I talked to @liangyuanruo about it, and am working on removing that eslint check. Edit: After a closer inspection, not all function calls ends after the response statements. An example is LoginController. A double response can be happen if an error is thrown when generating an otp. |
|
fired gitpod and did a grep project-wide grep check. confirmed that all relevant |
| if (error instanceof NotFoundError) { | ||
| res.unauthorized(jsonMessage('OTP expired/not found.')) | ||
| } else { | ||
| res.serverError(jsonMessage(error.message)) | ||
| return | ||
| } | ||
| res.serverError(jsonMessage(error.message)) | ||
| return |
There was a problem hiding this comment.
I think we should retain the else-if and else blocks here for greater code clarity.
| res.notFound(jsonMessage('User session not found')) | ||
| return |
| res.status(302).redirect(longUrl) | ||
| return |
| if (error instanceof AlreadyExistsError) { | ||
| res.badRequest(jsonMessage(error.message, MessageType.ShortUrlError)) | ||
| } else { | ||
| logger.error(`Error creating short URL:\t${error}`) | ||
| res.badRequest(jsonMessage('Server error.')) | ||
| return | ||
| } | ||
| logger.error(`Error creating short URL:\t${error}`) | ||
| res.badRequest(jsonMessage('Server error.')) | ||
| return |
| logger.error(`Error editing URL:\t${error}`) | ||
| res.badRequest(jsonMessage(`Unable to edit short link "${shortUrl}"`)) | ||
| return |
| if (error instanceof AlreadyOwnLinkError) { | ||
| res.badRequest(jsonMessage(error.message)) | ||
| } else { | ||
| logger.error(`Error transferring ownership of short URL:\t${error}`) | ||
| res.badRequest(jsonMessage('An error has occured')) | ||
| return | ||
| } | ||
| logger.error(`Error transferring ownership of short URL:\t${error}`) | ||
| res.badRequest(jsonMessage('An error has occured')) | ||
| return |
| res.serverError(jsonMessage('Error retrieving URLs for user')) | ||
| return |
|
Discussed with @liangyuanruo that the removal of else and else-if statements is because of an ESLint rule, and we will be keeping the rule for now. |
Problem
Some of the response statements are not followed by a return statement.
Solution
res.jsonstatement.