Preserving stacktrace when throwing Exception after SLAPing

[maxxng]

When we refactor code based on SLAP, do we catch and immediately throw an Exception? For example,

public void method() throws SomeException {
  if (isBadOutcome) {
    throw new SomeException();
  }

  ...
}

Is it okay to refactor this to

public void method() throws SomeException {
  try {
    handleBadOutcome(isBadOutcome);
  } catch (SomeException se) {
    throw se;
  }
  ...
}

private void handleBadOutcome(boolean isBadOutcome) throws SomeException {
  if (isBadOutcome) {
    throw new SomeException();
  }
}

Not sure if immediately rethrowing Exceptions is a good practice. Thanks!

[tlylt]

I think this is a great question and there's not much information in the textbook on the aspect of handling exceptions at which suitable level of abstraction. However, there is this statement from the textbook:

Most languages allow code that encountered an "exceptional" situation to encapsulate details of the situation in an Exception object and throw/raise that object so that another piece of code can catch it and deal with it. This is especially useful when the code that encountered an unusual situation does not know how to deal with it.

I believe in our program, we typically throw exceptions that are ultimately resolved at a higher level of abstraction. For example (AB3), the LogicManager has a method execute as follows:

public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }

There are two ways of handling exceptions displayed here. One is that it abstracted out a parseCommand method which throws ParseException. The execute method does not handle it but merely states that it throws ParseException, passing the work to yet another higher level of abstraction. In this case, it goes all the way up from LogicManager -> MainWindow -> CommandBox where the exception is handled as follows:

/**
     * Handles the Enter button pressed event.
     */
    @FXML
    private void handleCommandEntered() {
        String commandText = commandTextField.getText();
        if (commandText.equals("")) {
            return;
        }

        try {
            commandExecutor.execute(commandText);
            commandTextField.setText("");
        } catch (CommandException | ParseException e) {
            setStyleToIndicateCommandFailure();
        }
    }

This is a case where the author (I'm guessing here) believes that CommandBox is the right place (as in appropriate and justifiable) to resolve the two exceptions. Hence, the classes along the execution path could simply pass along the exception without dealing with it.

If you look at LogicManager's execute method again, it resolves another type of exception: IOException. This is a case where the author (I'm guessing here) believes that it is the job of LogicManager to handle the exception created when saving data into storage.

I believe that handling exceptions at the right level of abstraction is not an easy task. Let's see what Prof & others' insights are 👀

[kouyk]

Not sure if immediately rethrowing Exceptions is a good practice.

To quote from the textbook

The exception handler chosen is said to catch the exception.

Since exceptions can automatically propagate through the call stack, any method catching it should be in charge of handling it instead of throwing it again. In a similar fashion to the example @tlylt mentioned, the refactoring should be as follows:

public void method() throws SomeException {
    handleBadOutcome(isBadOutcome);
    ...
}

private void handleBadOutcome(boolean isBadOutcome) throws SomeException {
    // some attempts to recover before throwing exception, sets isBadOutcome to false if resolved
    if (isBadOutcome) {
        throw new SomeException();
    }
}

While using SLAP for this scenario, handleBadOutcome is more of a helper function of sorts to achieve SLAP and not exactly an abstraction in the sense of having different responsibility. In this case I believe we can just view both method like they are at the same level of abstraction and not see the original method as a higher level that should catch it.

[damithc]

As @kouyk @tlylt pointed out, the exception should be handled by the code that is most suited for the job -- this is based on the fact that the code that detects a problem may not know how to handle it. For example, the Storage component might detect that the target file is missing. Handling that problem involves informing the user about the missing file but the Storage component has no business interacting with the user. That means the exception need to be propagated to the Ui component for it to be handled there.

Another thing to note is that the code that is supposed to handle the exception should be given exceptions that it can understand. For the above example, the Ui might not (or should not) understand low-level exceptions such as 'I/O exception' -- hence, the exceptions given to it must be sufficiently high level such as 'could not write to the file'.

Following from the above, each level of abstraction must throw exceptions that match that level of abstraction e.g., it is OK for the parser to say 'invalid syntax' but probably not not OK for it to say 'tokenizing failed' (clients of the Parser should not be told anything about tokenizing as that is an internal low-level detail). However, 'tokenizing failed' is an appropriate exception to throw by the Tokenizer class. This means exception objects may have to be transformed into different other exceptions along the way as they get propagated through the call stack. This will need catch-transform-throw type code.

Since the OP @maxxng mentioned 'preserving stack trace' in the issue title, note that you can construct a new exception while embedding the original exception inside it (see constructors given in https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Exception.html) in case someone else needs to retrieve the original cause of the exception or the full stack trace.

[maxxng]

Thanks @tlylt @kouyk @damithc for the insights! Here is a quick summary (or at least my understanding, feel free to correct me!) based on the advice above: it seems that if the type of the Exception changes due to the difference in level of abstraction, it would be best for the code at a higher level of abstraction to catch and transform the Exception to an appropriate one that matches the current level of exception. Whereas, if the level of abstraction is the same (or the type of exception is the same) the code does not need to catch the exception and the Exception will automatically be handled. In addition, the stack trace will still be preserved when you a. Transform the exception, or b. let the method automatically handle the exception e.g

public void method() throws SomeException {
    handleBadOutcome(isBadOutcome);    // Note the lack of the try and catch block here.
    ...
}

private void handleBadOutcome(boolean isBadOutcome) throws SomeException {
    if (isBadOutcome) {
        throw new SomeException();
    }
}