Reduce Optionals to throw an error once

Question

I have the following response coming from a rest call and performing some logic based on what is returned.

This is what I want.

If the overall status code is NOT 200 OR If within the list of SimpleResponse, none of the SimpleResponse objects has a 200 httpCode, throw an error.

1. Too much going on there, having an optional within another optional.
2. And then throwing the same error at 2 different places.

Is there a cleaner way to write this?

These are the 2 related objects

@Getter
@Setter
public class SimpleResponses {
 private List<SimpleResponse> simpleResponsesList;
}
@Getter
@Setter
public class SimpleResponse {
 private String httpCode;
 // ... other fields
}

Method calling rest call and throwing error if needed.

public ResponseEntity<SimpleResponses> get() {
 HttpEntity<Object> httpEntity = this.getEntity();
 // restTemplate is from Spring
 ResponseEntity<SimpleResponses> responseEntity = restTemplate.exchange(url, HttpMethod.GET, httpEntity, SimpleResponses.class);
// START
// This is the logic to throw error depending on output as mentioned above.
// looking for a better way to write this. 
 // if none of the object inside the list has 200 code, throw error
 Optional.ofNullable(responseEntity.getBody())
 .map(SimpleResponses::getSimpleResponses)
 .ifPresent(response -> {
 Optional<SimpleResponse> simpleResponse = response.stream()
 .filter(responseStream -> responseStream.getHttpCode().equals("200"))
 .findAny();
 if (!simpleResponse.isPresent()) {
 throw new CustomRuntimeException("Failed ..... "); // repetitive same error being thrown again below.
 }
 });
 // if overall code is not 200, throw error too 
 if (!responseEntity.getStatusCode().is2xxSuccessful()) {
 throw new CustomRuntimeException("Failed ..... ");
 }
// END
 return responseEntity;
}

Answer 1

I'm not really addressing your main concerns, but a few things stood out to me about your code.

• It seems upside down. I haven't used restTemplate, however since the http status code is in the header, I'd assume that you can check it before trying to process the response body. Since the status code check is the most straightforward and a lot of codes would indicate that there is no body, I would tend to check this first, then go on to validate the body.

• When you're processing the body, you only throw if there is a list of responses, none of which are status "200". This matches your description of the requirements, but is it really true? How come "null response body", and "null simpleResponsesList" are acceptable? Why is this different to an "empty simpleResponsesList"?

• Throwing the same error from different locations in a method isn't necessarily bad, if it is the cleanest way to achieve your desired outcome. If I had a method with three parameters that needed validated, I might throw InvalidParameterException three times, once for each parameter, probably with the message tailored to that parameter. In your code it seems like a failure in the high level call may indicate something completely different (such as a network/server error) and a non 200 code from one of the nested responses. You don't include CustomRuntimeException, or all of SimpleResponse, but I can certainly picture a scenario where I might want to include different information such as something to identify the request which a failed SimpleResponse is associated with.

Answer 2

While it is possible to write the code in a functional manner the sequence of steps from the implementation makes it less obvious.

Here is an incremental approach to implement the one liner version of the code from description.

First implement the right sequence of transformations to facilitate the achievement of the aimed solution.

Optional.of(responseEntity)
 .filter(response -> response.getStatusCode().is2xxSuccessful())
 .map(response -> response.getBody().getSimpleResponses())
 .flatMap(simpleResponses -> simpleResponses.stream()
 .filter(simpleResponse -> "200".equals(simpleResponse.getHttpCode()))
 .findAny())
 .orElseThrow(() -> new RuntimeException("Failed..."));

Second factor out in the nesting line the the nested transformation.

Optional.of(responseEntity)
 .filter(response -> response.getStatusCode().is2xxSuccessful())
 .map(response -> response.getBody().getSimpleResponses())
 .orElseGet(() -> Collections.emptyList())
 .stream()
 .filter(simpleResponse -> "200".equals(simpleResponse.getHttpCode()))
 .findAny()
 .orElseThrow(() -> new RuntimeException("Failed..."));

Answer 3

There definitely is a way to refactor the functional non-functional code to a cleaner solution in this specific case where the mix is being awkward from readability and maintainability perspectives with the simplest approach of opting for a functional solution that gets the Optional from the responseEntity, filters by status code, flats to simpleResponse, filters by HTTP status code, gets a simpleResponse or else throw an exception being a possible solution presented sequentially in the following if the sequential presentation is preferred:

optional of responseEntity

filter by responseEntity's status code

flatMap to simpleResponses

filter by simpleResponse's HTTP status

orElseThrow an exception

A downside to consider to such a solution is the reduced ability to refine the thrown exception message.

Answer 4

You are misusing optionals. They were never meant to be a generic replacement for if-statements. They were specifically meant to convey the possibility of a null return value in public API's and to simplify hanling of nulls in fluent interfaces.