Imagine some code:

public void doSomething(Object object){
    try {
        if (object == null)
            throw new BusinessException("Object was null");
        
        try { 
            // do logic actions
        } catch (Exception e) {
            throw new BusinessException("Something went wrong doing logic", e)
        }

        try {
            // do some IO actions
        } catch (Exception e) {
            throw new BusinessException("Something went wrong doing IO.", e)
        }
        
    } catch(Exception e){
        throw new BusinessException("Something went wrong in doSomething.", e)
    }
}

The BusinessException is an extension of RuntimeException. I'm told by my manager and another senior engineer that BusinessException is the only exception that should ever get thrown and every method should be designed like the above method to ensure that. Anytime something goes wrong they want the same BusinessException thrown.

They idea is that they want to "abstract" away logical exceptions from the user and only provide "business exceptions" to the user. My manager does not want us to only catch specific exceptions, e.g. IOException They want to always catch(Exception) to make sure nothing is missed.

I don't understand this "abstraction" they are talking about. I'm pretty sure nothing is being "abstracted" away, an exception is just being encapsulated (or masked) in a new exception.

Semantics aside, I find this truly bizarre and I'm struggling to understand the value they think this verbose exception handling provides. It is not hard for me to imagine how this can make debugging more difficult. If any business exception gets thrown it immediately gets caught by another catch block and re-wrapped into a new exception, complicating the stack trace and potential debugging efforts.

It also seems like a performance issue to have so much exception instantiation.

Further, this is a spring boot application and we already have a ResponseEntityExceptionHandler

@ControllerAdvice
public class MyAppResponseEntityExceptionHandler extends ResponseEntityExceptionHandler{

    @ExceptionHandler(value = { IllegalArgumentException.class })
    protected ResponseEntity<Object> handleConflict(IllegalArgumentException ex, WebRequest request) {
        String bodyOfResponse = "Check the arguments";
        return handleExceptionInternal(ex, bodyOfResponse, new HttpHeaders(), HttpStatus.CONFLICT, request);
    }

    // Several more for different exception types...

}

Is this just a "to each their own" situation or is this objectively a problem?

CodePudding user response：

First of all, it's never recommended to have a generic catch block that catches and instance of Throwable or Exception, unless it exists in a chain of catch blocks, for example:

public void doSomething() {
    try {
        // do some database stuff
    } catch (SQLException e) {
        throw new BusinessException("something went wrong with database", e);
    }

    try { 
        // do some IO stuff 
    } catch (IOException e) {
        throw new BusinessException("something went wrong with IO");
    }
}

Now anything other than those two exceptions shouldn't be caught, since it's not the responsibility of this particular function, function should only complain about errors that are relative to what they do.

as a caller I might do something like this:

SomethingDoer s = new SomethingDoer();
s.doSomething();

now if I'm worried that an exception might get thrown unexpectedly, it's my responsibility as a caller to handle it, so the API deligates the uncaught exception for the caller to handle, like so:

SomethingDoer s = new SomethingDoer();
try {
    s.doSomething();
} catch ( BusinessException e) {
    LOGGER.error(e.message) // prod logging
    LOGGER.debug(e) // debug logging with stacktrace
    // hypothetical error listener
    errorListener.onError(e);
    //handle or log, but not rethrow.
} catch (Exception e) { // cringe..
    LOGGER.error("something went wrong, unexpectedly"); // prod logs
    LOGGER.debug("something went wrong, unexpectedly", e); // debug logs with stacktrace
  /* logged AND rethrown since up till this point all expected
exceptions should be wrapped and rethrown or logged, 
so if we get here its a fatal error, and you need to interrupt the application*/
    throw e;

The latter - cringe looking - catch( Exception e) block is also not recommended and the exception should be propagated up the stack to the main thread, Checked Exceptions are usually handled that way.

So language specific exceptions - internal - should be caught and wrapped in a BusinessException even before reaching the ControllerAdvice handler and this handler - since it is relatively close to the view layer of the app should only handle business specific exceptions and not internal exceptions.

CodePudding user response：

Your manager and senior engineer may be considering Effective Java. From the third edition, Item 73: Throw exceptions appropriate to the abstraction.

It is disconcerting when a method throws an exception that has no apparent connection to the task that it performs. This often happens when a method propagates an exception thrown by a lower-level abstraction. Not only is it disconcerting, but it pollutes the API of the higher layer with implementation details. If the implementation of the higher layer changes in a later release, the exceptions it throws will change too, potentially breaking existing client programs.
To avoid this problem, higher layers should catch lower-level exceptions and, in their place, throw exceptions that can be explained in terms of the higher-level abstraction. This idiom is known as exception translation.

Perhaps your manager is being overzealous with this bit of advice. Effective Java goes on to caution,

While exception translation is superior to mindless propagation of exceptions from lower layers, it should not be overused.

You may be justified in pointing out this overuse to your manager, but I suspect persuasion will be difficult. You can take some solace in Item 72: Favor the use of standard exceptions. I personally prefer that advice, and tend to avoid creating custom exceptions, but certainly other developers feel differently.

CodePudding user response：

It is like, the end user doesn't know what to do with exception so a generic exception will be better.

You can write diff custom exceptions for diff type of operations, like Database calls, api calls and return only one type of exception to the caller.

i.e You can define you custom exception like this.

public class BusinessException extends RuntimeException {
    
    private final ErrorCodes errorCode;
    private final Object body;
       
    public BusinessException(String message, ErrorCodes errorCode) {
        super(message);
        this.errorCode = errorCode;
        this.body = null;
    }
            
    public BusinessException(String message, ErrorCodes errorCode, Object body) {
        super(message);
        this.errorCode = errorCode;
        this.body = body;
    }
}

Where ErrorCodes is the enum that will be having the ErrorCodes like InternalError, EntityNotFound, Unauthorised.

Now you can use this custom exception, you will catch any exception in the application and throw this exception with proper error message and error code.

Something like this.

throw new BusinessException("Error while fetching some api data.", INTERNAL_SERVER_ERROR); 

or

throw new ServiceException("User is not authorised to perform operation.", UNAUTHORIZED);