CLOUDSTACK-10365: Change the "getXXX" boolean-related method names to…#2602
CLOUDSTACK-10365: Change the "getXXX" boolean-related method names to…#2602Kui-Liu wants to merge 1 commit intoapache:masterapache/cloudstack:masterfrom Kui-Liu:_booleanCopy head branch name to clipboard
Conversation
… "isXXX". These boolean-return methods are named as "getXXX". Other boolean-return methods are named as "isXXX". Considering there methods will return boolean values, it should be more clear and consistent to rename them as "isXXX".
DaanHoogland
left a comment
There was a problem hiding this comment.
I would never -1 this change but am not sure if it fulfils any of our needs
| } | ||
|
|
||
| public boolean getEnableRRS() { | ||
| public boolean isEnableRRS() { |
There was a problem hiding this comment.
what is the is prefix for in this case? is the question enableRRS() ?
There was a problem hiding this comment.
Does not enableRSS give the idea that it is a method that "enables" RSS?
At least, that is what sounds to me.
| * @return Does this service plan offer HA? | ||
| */ | ||
| boolean getOfferHA(); | ||
| boolean isOfferHA(); |
There was a problem hiding this comment.
the java doc implies it should be called doesOfferHA()
There was a problem hiding this comment.
@DaanHoogland I would prefer to only have methods starting with either get or is.
Any other method name in a VO would not be usable for filtering,
according to the implementation of the VO MethodInterceptor
There was a problem hiding this comment.
good point @fmaximus so the generic dao breaks if we go any other way is your message, is it?
There was a problem hiding this comment.
Do you know the name of the class that does this interception, so we can inspect it?
Is it using method names or POJO's properties directly via their annotation?
| * @return Does this service plan support Volatile VM that is, discard VM's root disk and create a new one on reboot? | ||
| */ | ||
| boolean getVolatileVm(); | ||
| boolean isVolatileVm(); |
There was a problem hiding this comment.
supportVolitileVMs() would be more pleasing on the human eye. Not that that should hamper this change but what is the objective? to adhere to another naming convention or to make better code for reading? and seriously both are valid motives under circumstances, but I am really wondering.
There was a problem hiding this comment.
The intention of my pull requests is to make code more readable and understandable, thus further making developers to understand the code more easily with method names without more context information.
But about method naming things, developers have two different opinions as you mentioned:
(1) keep the same naming convention as similar methods.
(2) make code more readable.
There was a problem hiding this comment.
@brucekuiliu I am with 2 all the way, with the exception of the use of these methods in code generation tools. In this last method I would expect a name like isForVolatileVms() or the ealier sugested name. Of cause the accessor is named after the field implying that the field is not properly named either.
I am +0 on this one. Try to find more reviews.
|
@brucekuiliu are you still with us? your branch is out off date, can you rebase? |
|
@DaanHoogland Yes. I removed all my branches. I need to create a new pull request. |
|
That's fine. at your own time ;) |
|
closing in favour of #2816 |
… "isXXX".
These boolean-return methods are named as "getXXX".
Other boolean-return methods are named as "isXXX".
Considering there methods will return boolean values, it should be more clear and consistent to rename them as "isXXX".