Is Exception caught in the Service class a matter of preference?

Question

In a Java EE legacy project, almost all the DAO and Service classes are written in a a way that DAO level does not catch any exception and instead the service classes catch(Exeption e) in all of their methods. The following is an example when adding a new method to the DAO and Service

A new find method, findByStatus, is a legacy DAO class AccountDAO

public List<Account> findByStatus(Status status) {
    return entityManager.createQuery(
                    "FROM Account a WHERE a.status = :status"
            )
            .setParameter("status", status)
            .getResultList();
}

And then it is used in a service class

public List<Account> findByStatus(Status status) {
    List<Account> retVal = new ArrayList<>();
    try {
        retVal = accountDAO.findByStatus(status);
    } catch (Exception e) {
        LOG.error("Error in findByStatus", e);
    }
    return retVal;
}

The code above can work since the code has been in production for more than ten years, but

QUESTION: Is this a matter of preference to handle errors on the DAO or service level? And should the new method be added in the same way, i.e. letting service classes catch(Exception e) instead of ignoring any exception in the DAO class level, for the sake of consistency with legacy code?

Answer 1

Is this a matter of preference to handle errors on the DAO or service level?

No, it is not a matter of preference, because the DAO level will probably not even know about the service's LOG, so it cannot just handle the exception in an equivalent way.

Whether it is a good idea to catch the exception in the service level (and swallow it), to catch it for logging and rethrow it, or if it should be caught exclusively in some higher application level is debatable, but currently your question presents us a false dichotomy.

To make an informed decision about the correct layer for catching an exception we would need to understand what kind of errors can actually cause an exception in the DAO. When, for example, these exceptions signalize a wrong SQL statement, that could be a reason to end the whole application - so just logging and continue will be the wrong action. When, for example, this exception can be caused by a database or network disconnection, we would need to know about the global failover mechanisms in this application for deciding where to catch it (maybe for initiating a reconnect).

Or when some of your DAO functions throw certain kind of exceptions for things which can be handled at the service or application in a sensible way, then the service handler might catch and handle it. Still, I would not expect the service layer to implement a "catch all" handler catch(Exception e)..., but a catch(MySpecificExceptionIKnowICanHandle e).... The only exception here is when the service layer logs and rethrows everything it cannot handle by itself.

So when it comes to the question doing things for reason of consistency:

• when all your services log exceptions to some log file, it seems reasonable to implement a new function with the same logging mechanics

• if your service should swallow the exception then afterwards, or let it bubble up by rethrowing, is nothing you decide in a cargo-cult manner. You decide it by informing yourself in which context the new service-function findByStatus will be used, whether it is some legacy code where you have no chance to change any missing exception handling, or if it is used in some code where proper exception handling can be added.

Answer 2

Everything is a matter of preference.

But the code you've shown is pretty much a perfect example of how not to use exceptions - other than in extreme cases (e.g. a top-level handler just before the application quits), you shouldn't be catching Exception (instead catch the actual exceptions you can handle) and you shouldn't be just logging it and continuing (in your code, you can't tell the difference between "error" and legitimate "no records found"): if you can't handle an exception, let it bubble up to somewhere that does have enough context to handle it.

However, acknowledging that this is legacy code I know it could take a lot of work to put in proper exception handling, even on a piecemeal basis. You/your team/your employer need to make a decision as to whether that's a good investment to make in this project.

Answer 3

The code above can work since the code has been in production for more than ten years, but [..]

Anything that you do beyond getting the code to work is a matter of preference, if getting the code to work is your only goal. I'm aware you're asking a good question with an intent of good practice, which I commend, but I want to point out that "matter of preference" is not the counter to a good practice suggestion. Technically, everything is a matter of preference. But that should not be the driving decider on whether you accept it or not.

This is a subjective topic, not because of a personal preference, but rather because of the contextual nature of the specific exception you're dealing with.

The goal in deciding where to catch an exception is to figure out who is meaningfully capable of handling said exception, which is a different question for every different exception.
There are times when you can handle an exception immediately. There are times when you shouldn't handle an exception at all, instead letting it bubble up all the way to the top. There are times where a man in the middle is the right one to respond to an exception.

catch(Exeption e) in all of their methods

Often called Pokémon exception handling. Overall, it's an antipattern.

There are cases where you want to handle and hide all exceptions, but those are generally right at the very edge, e.g. in the context of an HTTP backend it should only happen at the framework level when an exception bubbles out of the endpoint's method body.

Doing it in your application logic, and doing it blindly (i.e. all exceptions, no matter the exception) is indicative of poorly thought out design.

And should the new method be added in the same way

I strongly advocate for a "clean as you go" methodology where you iteratively implement better code as you work on the codebase.

However, you have to balance this against the benefit of having a consistent codebase. Even if it's not perfect, it's often better to have a codebase that consistently tackles the same problem the same way, instead of being a patchwork of different developers (in different times) implementing the same thing differently.

If there is no appetite to improve the old code, not even eventually, then don't bother doing things differently now. I strongly recommend you instead commit to cleaning up all the code as you go; but I cannot force you to do so.

Answer 4

When an exception/problem occurs, you basically have three choices:

• Suppress it.
• Handle it.
• Transform/Pass it.

If you catch an exception and do nothing, most people would say you suppressed it. However if you log the fact the problem occurred, increment an error metric then return an empty list, it is possible there really isn't any other action that makes sense, so some would say that in this scenario you "handled" the problem.

Within a method you may choose not to handle a problem, this may be as simply as letting it fall through, declaring the method throws the exception or transforming it in some way:

• Catching the original exception and:
  • Throwing a different exception.
  • Returning an error code.
  • Returning a special value (null) to indicate an error
• Substituting a special value (null) by throwing an exception.

The commonality in all of these scenarios, is that there is an expectation that some logic higher up the stack will check for the error condition and deal with it - i.e. processing of the error has not yet been completed when the method returns.

All this can be generalized to two scenarios: Further processing of the problem is required or it isn't.

---

Is this a matter of preference to handle errors on the DAO or service level?

Transforming exceptions to/from other exceptions and/or status codes is a matter of style/choice, generally you transform the problem condition when it makes the overall code more readable or makes it easier to write logic to handle the conditions. Transformations may make sense at any layer.

Where you chose to handle a problem is often forced upon you, in that you need to propagate the condition far enough through the call stack that it allows you to skip over some logic and/or substitute custom logic to cleanup and allow processing to continue.

Another option is to propagate the error all the way back to the top level:

• For a console app that may mean the application crashes / aborts all future processing.
• For a GUI app that may mean the action (that the user requested) fails.
• For a Web app the whole request fails.

Should the new method be added in the same way, i.e. letting service classes catch(Exception e) instead of ignoring any exception in the DAO class level, for the sake of consistency

Generally you want to report errors back to the user or system that initiated the request, you should think carefully if it really makes sense to substitute an empty result-set and allow processing to continue.

Two exceptions to this are:

• Batch processing - where you want the rest of the batch to continue processing even if one item fails - although you will probably report 99/100 successful and provide details about the failed record.
• Graceful degradation - your business requirements may state the some functionality can/should be sacrificed, hence you will need to substitute a default value for any components that fail.

Answer 5

It is particularly relevant to code management and the way you want to act when an exception occurred.

I'm working on a project and recently encountered this exact thought and discussed with team to how efficiently we can handle exception, so one way after looking over-the-internet we've found that introduce an handler that handles the exception and logs the correct exception in correct context.

At DAO Level, we don't catch any exception since there's unlikely to be an exception to be occurred at all (in Spring Boot's Repositories then not at all except like when you wanted to find All X by Y and you do it like this findAllXbyZ where Z doesn't exists thden Spring Boot won't boot the application since it'll throw an exception at building-time)

and at Service-Level, we only catch business exception only where it needs else no!