aws-cloudformation / cloudformation-cli-java-plugin

The CloudFormation Provider Development Toolkit Java Plugin allows you to autogenerate java code based on an input schema.
Apache License 2.0
30 stars 47 forks source link

[Proposal] Use unboxed boolean types in ResourceHandlerRequest #341

Open osdrv opened 3 years ago

osdrv commented 3 years ago

There are 2 recent changes that landed in ResourceHandlerRequest: https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/commit/ebca1dc12e187b484ed599db65d726a6b23a14c0#diff-adfb207dac8dff5107b68b36280ccad4525f92176184ad2ffff9aa7e591b2e8a and https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/commit/e34f0472d0bc67953f41d4ceb666a175d0a7fb2d#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.

pemmasanikrishna commented 3 years ago

@ammokhov have raised pr https://github.com/aws-cloudformation/cloudformation-cli-java-plugin/pull/365

wbingli commented 3 years ago

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 commented 3 years ago

@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?