0
\$\begingroup\$

I have this piece of code placed in ESI class. There is switch statement nested in a try-catch block and it feels simply wrong for me (I have a suspicion that I'm close to exception driven development). I'm looking forward to read your suggestions on how could I potentially improve this code.

@PostConstruct
public void initialize() {
    this.webTarget = this.client.target("http://demo1173642.mockable.io/");
}

public Message getMockMessage() {
    Response response = null;

    try {
        response = this.webTarget.request().get();

        switch (response.getStatusInfo().getFamily()) {
            case SUCCESSFUL:
                return response.readEntity(Message.class);

            case SERVER_ERROR:
                throw new WebApplicationException(ErrorCode.EXTERNAL_SERVICE_BAD_GATEWAY.getMessage(),
                        Response.Status.BAD_GATEWAY);

            default:
                throw new InternalServerErrorException(ErrorCode.EXTERNAL_SERVICE_EXCEPTION.getMessage());
        }

    } catch (Exception ex) {
        logger.log(Level.WARNING, ErrorCode.UNEXPECTED_EXECPTION.getMessage(), ex);
        if (response != null) {
            response.close();
        }
        throw new InternalServerErrorException();
    }
}
\$\endgroup\$
1
\$\begingroup\$

In fact, your error handling is quite messed up: in the cases for server error and default you throw an exception, which you immediately replace with a different exception in your own catch block. I don't think that's what you meant to do. Furthermore, you only close() the response object in the exception case, but not in the success branch...

Nevertheless, the throwing of a WebApplicationException leads me to believing that we are in a RESTful application here. Thus, the best way would be to declare an excepton mapper to handle the underlying ProcessingException (which is the only one your catch-all-block should encounter) and do not do the handling internally. For exception mappers, here's a tutorial to start with: https://dennis-xlc.gitbooks.io/restful-java-with-jax-rs-2-0-en/cn/part1/chapter7/exception_handling.html (but you will find a lot more resources using your favourite search engine.)

Having this out of the way, the only thing for your method to do is using a try-with-resources block for the response:

public Message getMockMessage() {
    try(Response response = this.webTarget.request().get()) {
        switch (response.getStatusInfo().getFamily()) {
            case SUCCESSFUL:
                return response.readEntity(Message.class);
            case SERVER_ERROR:
                throw new WebApplicationException(ErrorCode.EXTERNAL_SERVICE_BAD_GATEWAY.getMessage(),
                        Response.Status.BAD_GATEWAY);
            default:
                throw new InternalServerErrorException(ErrorCode.EXTERNAL_SERVICE_EXCEPTION.getMessage());
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your comprehensive response. Nevertheless, I'm afraid that you did not fully grasp the intention of the code. First of all - readEntity() closes stream after successful read, so closing it once again by hand is not required Second thing - yes, I do replace the exception but first I log the exception from the another microservice for debugging purposes. Exception outside this microservice has code 502 which propagates inside my system with 500 code. This behaviour I'd like to keep. \$\endgroup\$ – miki Aug 27 '18 at 7:37
  • \$\begingroup\$ Btw. read error messages represented by ErrorCode enum. Did you think that these are purposeless? \$\endgroup\$ – miki Aug 27 '18 at 7:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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