In this function that handles a REST API call, any of the called functions to handle parts of the request might throw an error to signal that an error code should be sent as response. However, the function itself might also discover an error, at which point it should jump into the exception handling block.
static async handleRequest(req) {
try {
let isAllowed = await checkIfIsAllowed(req);
if (!isAllowed) {
throw new ForbiddenException("You're not allowed to do that.");
}
let result = await doSomething(req); // can also raise exceptions
sendResult(result);
} catch(err) {
sendErrorCode(err);
}
}
Webstorm will underline the throw with the following message: 'throw' of exception caught locally. This inspection reports any instances of JavaScript throw statements whose exceptions are always caught by containing try statements. Using throw statements as a "goto" to change the local flow of control is likely to be confusing.
However, I'm not sure how to refactor the code to improve the situation.
I could copypaste the code from the catch block into the if check, but I believe this would make my code less readable and harder to maintain.
I could write a new function that does the isAllowed check and throws an exception if it doesn't succeed, but that seems to be sidestepping the issue, rather than fixing a design problem that Webstorm is supposedly reporting.
Are we using exceptions in a bad way, and that's why we're encountering this problem, or is the Webstorm error simply misguiding and should be disabled?
You're checking for something and throwing an exception if isAllowed fails, but you know what to do in that situation - call sendErrorCode. You should throw exceptions to external callers if you don't know how to handle the situation - ie in exceptional circumstances.
In this case you already have a defined process of what to do if this happens - just use it directly without the indirect throw/catch:
static async handleRequest(req) {
try {
let isAllowed = await checkIfIsAllowed(req);
if (!isAllowed) {
sendErrorCode("You're not allowed to do that.");
return;
}
let result = await doSomething(req); // can also raise exceptions
sendResult(result);
} catch(err) {
sendErrorCode(err);
}
}
I could copypaste the code from the
catchblock into theifcheck, but I believe this would make my code less readable and harder to maintain.
On the contrary, as above, I would expect this to be the way to handle this situation.
Contrary to James Thorpe's opinion, I slightly prefer the pattern of throwing. I don't see any compelling reason to treat local errors in the try block any differently from errors that bubble up from deeper in the call stack... just throw them. In my opinion, this is a better application of consistency.
Because this pattern is more consistent, it naturally lends itself better to refactoring when you want to extract logic in the try block to another function that is perhaps in another module/file.
// main.js
try {
if (!data) throw Error('missing data')
} catch (error) {
handleError(error)
}
// Refactor...
// validate.js
function checkData(data) {
if (!data) throw Error('missing data')
}
// main.js
try {
checkData(data)
} catch (error) {
handleError(error)
}
If instead of throwing in the try block you handle the error, then the logic has to change if you refactor it outside of the try block.
In addition, handling the error has the drawback of making you remember to return early so that the try block doesn't continue to execute logic after the error is encountered. This can be quite easy to forget.
try {
if (!data) {
handleError(error)
return // if you forget this, you might execute code you didn't mean to. this isn't a problem with throw.
}
// more logic down here
} catch (error) {
handleError(error)
}
If you're concerned about which method is more performant, you shouldn't be. Handling the error is technically more performant, but the difference between the two is absolutely trivial.
Consider the possibility that WebStorm is a bit too opinionated here. ESLint doesn't even have a rule for this. Either pattern is completely valid.
Since this is not a blocking error, but only an IDE recommendation, then the question should be viewed from two sides.
The first side is performance. If this is a bottleneck and it is potentially possible to use it with compilation or when transferring to new (not yet released) versions of nodejs, the presence of repetitions is not always a bad solution. It seems that the IDE hints precisely in this case and that such a design can lead to poor optimization in some cases.
The second side is the code design. If it will make the code more readable and simplify the work for other developers - keep it. From this point of view, solutions have already been proposed above.