Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

Conversation

@seongahjo
Copy link

Hi I'm glad to find best graphql library for java.
I think in AbstratGraphQLController "if statement" is bad for readability in such simple condition.
It is better to use ternary operator.

Copy link
Member

@oliemansm oliemansm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

if (body == null) {
body = "";
}
body = body != null ? body : "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a null check you could also do body = Optional.ofNullable(body).orElse(""); Although now that I look at this apparently we're re-assigning a parameter here, which isn't good practice to begin with. Besides that it's a pretty strange re-assignment too, since in my opinion it would probably be cleaner if the methods this value is passed to below are able to cope with a null value. That would eliminate the need for this re-assignment completely.

return Collections.emptyMap();
}
return objectMapper.deserializeVariables(jsonMap);
return jsonMap != null ? objectMapper.deserializeVariables(jsonMap) : Collections.emptyMap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be written as

return ofNullable(jsonMap).map(objectMapper::deserializeVariables).orElseGet(emptyMap);

But I guess that's just a matter of personal preference :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I find that your other codes are written as optional.
I think it is better in this case.
Thank you.

refactor if statement into ternary operator
@oliemansm oliemansm merged commit 532e04b into graphql-java-kickstart:master Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants