Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Use unboxed boolean types in ResourceHandlerRequest #341

Open
osdrv opened this issue Dec 17, 2020 · 3 comments
Open

[Proposal] Use unboxed boolean types in ResourceHandlerRequest #341

osdrv opened this issue Dec 17, 2020 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@osdrv
Copy link

osdrv commented Dec 17, 2020

There are 2 recent changes that landed in ResourceHandlerRequest: ebca1dc#diff-adfb207dac8dff5107b68b36280ccad4525f92176184ad2ffff9aa7e591b2e8a and e34f047#diff-adfb207dac8dff5107b68b36280ccad4525f92176184ad2ffff9aa7e591b2e8a.

Both changes favor boxed Boolean over primitive types.

As a result, the usage of this class implies on importing BooleanUtils or a similar helper in order to avoid constructions like: if (request.getSnapshotRequested() != null && request.getSnapshotRequested() == true) { ... }.

And the code that is performing a "naive" boolean comparison like: if (request.getSnapshotRequested()) { ... } will fail with a null-pointer exception.

The same stands for the rollback flag.

I wonder if var type boxing was done for some specific reasons? If there are no strict reasons, I would vote for unboxing them to simplify condition checking.

@ammokhov ammokhov added enhancement New feature or request good first issue Good for newcomers labels Jan 19, 2021
pemmasanikrishna pushed a commit to pemmasanikrishna/cloudformation-cli-java-plugin that referenced this issue Apr 10, 2021
@pemmasanikrishna
Copy link

@ammokhov have raised pr #365

@wbingli
Copy link
Contributor

wbingli commented Apr 13, 2021

We use boxed type because the property could not exist from the request, therefore, it could be null in such case.

It makes sense that if we know the default value of those boolean properties, then it's better to provide an extra helper methods on ResourceHandlerRequest. e.g ResourceHandlerRequest#isSnapshotRequested

@pemmasanikrishna
Copy link

pemmasanikrishna commented Apr 19, 2021

@wbingli if my understanding is correct, just providing helper methods that return primitive types to support backward compatibility instead of changing the data type of the actual instance variable is suggested. But what would the default values of these snapshotRequested, rollback and driftable be? false?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants