Is this exception handling missing anything?

Question

This is the code. I'm uncertain about the last block.

@Override
public List<PendingOrders> pendingOrder(PendingOrderRequest request, String source, String trackingId) throws CashHubException {
    logger.info("Initiated method: pendingOrder");
    RegisterPaymentsResponse registerPaymentsResponse = new RegisterPaymentsResponse();
    List<PendingOrders> orderPlansResponse = null;
    try {
        orderPlansResponse = paymentDao.pendingOrder(request);
    } catch (DataAccessException exe) {
        logger.error("Exception Occurred in PaymentServiceImpl pendingOrder ", exe);
        throw new OrderProcessingException("Database error occurred", exe);
    } catch (InvalidRequestException exe) {
        logger.error("Exception Occurred in PaymentServiceImpl pendingOrder ", exe);
        throw new InvalidRequestException("Invalid request", exe);
    } catch (RecordNotFoundException exe) {
        logger.error("Exception Occurred in PaymentServiceImpl pendingOrder: ", exe);
        throw new RecordNotFoundException("Refund processing error");
    } catch (Exception exe) {
        logger.error("Exception Occurred in PaymentServiceImpl pendingOrder ", exe);
        throw new OrderProcessingException("Order processing error", exe);
    }
    logger.info("Exiting method: pendingOrder");
    return orderPlansResponse;
}

Answer 1

You should only catch the exception declared on the function itself. Anything else is inappropriate. For example: you have catch (Exception e) but this literally includes all sorts of things you'd never want to catch (like RuntimeExceptions such as NullPointerException and ArrayIndexOutOfBoundsException. These are programatic mistakes and you wanna know exactly where those occurred so you can fix them.

Only catch what is explicitly declared. Let everything else float up.

Answer 2

The info level log statements should be logged on trace. As it is now it floods the log with meaningless entries. Entering and leaving a method aren't very interesting events. You should already know when those happen in your code flow. If it's for debugging, then learning to use a debugger and break points are a better alternative.

Logging and throwing an exception should never be the normal operation. It creates a duplicate stack trace on the log, which fills the log with unnecessary entries and is quite annoying to follow when debugging production issues. Also the first stack trace is incomplete because it doesn't include the caller. Logging should be handled by the code that stops the exception from propagating. The reason for deviating from this rule should be given in code comments.

Log messages that read "Exception occurred" are redundant and not very helpful. We know that there was an exception because there's a stack trace attached to the log entry. Instead try to explain what caused the error.

Catching the raw Exception is always a code smell. You should explain in the code comments why you don't know all the exception types that can be thrown by the code block. "Just to be sure" isn't an acceptable reason. Fixing this kind of code smells are quite difficult after they have been allowed to "rot" for a few years.

Answer 3

• Logging on method entry and exit should be done as TRACE, logger.trace(...). Typically for development. Maybe it is done as INFO here because this is hot code being developed.

• Often people turn checked Exceptions (needing throws/try-catch) into RuntimeExceptions. This is also typical, though not always needed. Here it seems that "internal" exceptions are rethrown as more public API exceptions. I think error codes and messages could be more versatile, especially as the new exception's cause still is internal and hence probably does not expose the underlying faulty data.

• However when you throw an exception, you should not log it there should it not be the final catch. The final catch should do any logging. For instance RecordNotFoundException not necessarily points to an error. You would pollute and hence disarm an error log.

• Catching a general Exception or maybe even RuntimeException might be justifiable at the APIs boundary to catch programming problems. However generally much information should be gathered, possibly even login info & time stamp. Everything that help solving the error. If the exception regularly occurs and nothing is done with it, then this is a code smell, a huge technical debt. So it should not be shown as an innocent but as a blocking (?) error.

In general one does not want to create a growing number of these custom exceptions over time.