How to fix "'throw' of exception caught locally"?

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?

Solution 1:

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 catch block into the ifcheck, 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.

Solution 2:

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.

Solution 3:

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.

Solution 4:

Return a promise reject instead of throwing error in the try block

  try {
    const isAllowed = await checkIfIsAllowed(request);

    if (!isAllowed) {
      return Promise.reject(Error("You're not allowed to do that."));
    }

    const result = await doSomething(request);

    sendResult(result);
  } catch (error) {
    throw error;
  }

Solution 5:

There are good answers to the question "Why not use exceptions as normal flow control?" here.

The reason not to throw an exception that you will catch locally is that you locally know how to handle that situation, so it is, by definition, not exceptional.

@James Thorpe's answer looks good to me, but @matchish feels it violates DRY. I say that in general, it does not. DRY, which stands for Don't Repeat Yourself, is defined by the people who coined the phrase as "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system". As applied to writing software code, it is about not repeating complex code.

Practically any code that is said to violate DRY is said to be "fixed" by extracting the repeated code into a function and then calling that function from the places it was previously repeated. Having multiple parts of your code call sendErrorCode is the solution to fixing a DRY problem. All of the knowledge of what to do with the error is in one definitive place, namely the sendErrorCode function.

I would modify @James Thorpe's answer slightly, but it is more of a quibble than a real criticism, which is that sendErrorCode should be receiving exception objects or strings but not both:

static async handleRequest(req) {
    try {
        let isAllowed = await checkIfIsAllowed(req);
        if (!isAllowed) {
            sendErrorCode(new ForbiddenException("You're not allowed to do that."));
            return;
        }
        let result = await doSomething(req); // can also raise exceptions
        sendResult(result);
    } catch(err) {
        sendErrorCode(err);
    }
}

The larger question is what is the likelihood of the error and is it appropriate to treat !isAllowed as an exception. Exceptions are meant to handle unusual or unpredictable situations. I would expect !isAllowed to be a normal occurrence that should be handled with logic specific to that situation, unlike, say, a sudden inability to query the database that has the answer to the isAllowed question.

@matchish's proposed solution changes the contract of doSomethingOnAllowedRequest from something that will never throw an exception to something that will routinely throw an exception, placing the burden of exception handling on all of its callers. This is likely to cause a violation of DRY by causing multiple callers to have repetitions of the same error handling code, so in the abstract I do not like it. In practice, it would depend on the overall situation, such as how many callers are there and do they, in fact, share the same response to errors.