ResponseEntity<CommonResponseDTO> commonResponseDto = testClient.getCategoryById(sampleEntity.getCategoryId());
if(Objects.nonNull(commonResponseDto) && Objects.nonNull(commonResponseDto.getBody()) && Objects.nonNull(commonResponseDto.getBody().getName())){
SuperDataDto superDataCategoryDto = SuperDataDto.builder().id(sampleEntity.getCategoryId()).name(commonResponseDto.getBody().getName()).build();
In the above code SonarQube complains that
A "NullPointerException" could be thrown; "getBody()" can return null. on
.name(commonResponseDto.getBody().getName())
I am even checking the nonNull
for commonResponseDto.getBody().getName())
. Am I doing anything wrong?
SonarQube is right.
if(Objects.nonNull(commonResponseDto)
&& Objects.nonNull(commonResponseDto.getBody())
&& Objects.nonNull(commonResponseDto.getBody().getName())) {
SuperDataDto superDataCategoryDto = SuperDataDto.builder().id(sampleEntity.getCategoryId()).name(commonResponseDto.getBody().getName()).build();
Makes no guarantee about the return value of getBody()
of consecutive calls. Here's an implementation that would easily make your condition throw:
private int calls = 0;
public Body getBody() {
++calls;
if (calls % 2 == 0) return null;
return new Body(...);
}
The first check is non-null, calling it a second time will return null and your code throws an exception. Don't rely on methods to return the same reference from multiple calls, if you require a single value, be explicit about it and store it in a local variable:
if (Objects.nonNull(commonResponseDto)) {
final CommonResponseDTO body = commonResponseDto.getBody();
if (Objects.nonNull(body)) {
final String name = body.getName();
if (Objects.nonNull(name)) {
SuperDataDto superDataCategoryDto = SuperDataDto.builder()
.id(sampleEntity.getCategoryId())
.name(name)
.build();
Note that Objects#nonNull
is mainly intended to be used as a method reference in lambda expressions and your code could be clearer if you simply wrote obj != null
.
But this becomes ugly quickly. You can increase the clarity of your code by moving it to a small helper function:
private static String nameOfResponse(
final ResponseEntity<CommonResponseDTO> response) {
if (response == null) return null;
final CommonResponseDTO body = commonResponseDto.getBody();
if (body == null) return null;
return body.getName();
}
// ...
final String name = nameOfResponse(commonResponseDto);
if (name != null) {
final SuperDataDto superDataCategoryDto = SuperDataDto.builder()
.id(sampleEntity.getCategoryId())
.name(name)
.build();
}
An alternative with a little bit of overhead is wrapping your object in an Optional
and then transforming this optional value:
Optional.ofNullable(commonResponseDto)
.map(CommonResponseDTO::getBody)
.map(Body::getName())
.ifPresent(name -> {
SuperDataDto superDataCategoryDto = SuperDataDto.builder()
.id(sampleEntity.getCategoryId())
.name(name)
.build();
});
But please note that it is discouraged to use Optional for flow control. They should only be used as return type of methods to signal the absence of a value.