2
\$\begingroup\$

I wrote the following method which would just call an API which would then update a user's subscription to something.

void updateSubscription(String userId, boolean subscribed){

        try{
            // some stuff
            restTemplate.exchange(.....)
           // some stuff
        }catch (HttpClientErrorException exception){
            log.error("API EXCEPTION - Received Status: {} - Stack Trace:",exception.getStatusCode(), exception);
            throw new ExternalAPIException("Service unavailable", HttpStatus.SERVICE_UNAVAILABLE);
        }catch (Exception e){
            log.error("API EXCEPTION - Stack Trace:", e);
            throw new ExternalAPIException("Internal Server Error", HttpStatus.INTERNAL_SERVER_ERROR);
        }
    }

I have two questions in particular:

  1. Is the void return type okay? I am actually leaning towards returning a boolean indicating if the request was successful.
  2. Is the error handling okay?

Note - it is a service class without a controller, so I can't use @controller's advice.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Welcome to the Code Review Community. Please post the complete code of the function so that we have enough information to properly review it. The comments // some stuff make the question off-topic for code review. Please read our guidelines on how to ask a good question. \$\endgroup\$
    – pacmaninbw
    Nov 10, 2020 at 13:25
  • \$\begingroup\$ What do you mean by "@controller's advice"? Is "controller" a user here? Referring to a now-deleted comment? \$\endgroup\$ Nov 10, 2020 at 16:45
  • 1
    \$\begingroup\$ @peter-mortensen it's the @Controller annotation (spring) \$\endgroup\$
    – Doi9t
    Nov 10, 2020 at 17:49

2 Answers 2

3
\$\begingroup\$

"okay" in programming often depends on context.

You chose to perform error handling by throwing exceptions, so there is no point in having a boolean return value - there will be no case where false is returned.

If you are asking whether it is better to return false rather than throw exception, that largely depends on the situation.

Your current error handling is already more granular than just success / failure - you want to detect different types of errors and respond differently, so the exception method is more appropriate.

Note that if you were to switch to return and maintain same granularity, you would need to use an Enum of possible results.

As for "Is error handling ok" - again, that depends. How much 'some stuff' do you have around the API call? What kind of stuff is it?

You may want to limit the amount of code in the catch block, and have separate catch blocks for other parts of code that may fail independently of any server errors.

Then again, how granular do you want to make your error handling? Usually, the more precise the error and the more information user of the function has regarding why it failed the better, but if there are some constraints or type of errors that are not important they may be omitted, consolidated, or handled silently.

\$\endgroup\$
3
\$\begingroup\$

If you're already using exceptions for error handling, adding an ok/fail return status alongside it adds no more additional useful information. Even if it was an error code structure with status and message, it adds nothing that is not already provided in the standard Java exception handling mechanism. You would just lose the stack trace information and exception chaining.

Adding a separate domain specific error reporting mechanism places an additional burden to the caller as they have to wade through your JavaDocs (which, if my industry experience is anything to take from, are most likely missing :)) to figure out how the status code should be handled. So I would rather not try to bury the exception handling logic inside a specialized error reporting mechanism. Especially as you will eventually run into more exceptions again down the stream, so whatever you achieve with your solution would be just a temporary win.

Also in this particular case, changing the method return value to ok/fail requires the caller to implement two different error handling routines for the same method call. So in this particular case it is absolutely not okay.

\$\endgroup\$
1
  • \$\begingroup\$ Are you referring the throwing of ExternalApiException as specialised error handling? \$\endgroup\$
    – lynxx
    Nov 10, 2020 at 13:41

Not the answer you're looking for? Browse other questions tagged or ask your own question.