-
Notifications
You must be signed in to change notification settings - Fork 24
Fixed MonetaryAccountReferenceTypeAdapter to output proper value. (bunq/sdk_java#49) #68
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fixed MonetaryAccountReferenceTypeAdapter to output proper value. (bunq/sdk_java#49) #68
Conversation
@patrickdw1991 all yours please 👀 |
out.nullValue(); | ||
} else if (value.getPointer() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be nicer to check if the value.getLabelMonetaryAccount
is not null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But thats already done above ? if (value == null || value.isAllFieldNull()) {
via isAllFieldNull
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The statements that follow is purely to know which one to use as both cant be populated with data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There you check if all fields are null. Here you check if the pointer is null. Wouldn't it be nicer to check:
else if (value.getLabelMonetaryAccount() != null) {
Because in theory it doesn't mean that if not all fields are null and the pointer is null that the Monetary account label is not null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮 😁 🤦♂️ makes sense.
@andrederoos all yours please 👀 |
This PR closes/fixes the following issues: