qmcpack icon indicating copy to clipboard operation
qmcpack copied to clipboard

Accessor naming convention

Open PDoakORNL opened this issue 5 years ago • 3 comments

While I thought we did currently we don't specify a specific format for accessors. An exception for data member accessors is useful, it is useful for both searching the code and ease of reading.

I propose when a method is a simple accessor i.e. a get or set that we avoid

VariableType getVariableName() const { return variable_name_; }

in favor of

VariableType get_variable_name() const { return variable_name_; }

In the case instances where the accessor results in a calculation and the member variable serves as a cache of the answer the normal convention holds.

VariableType getVariableName { if ( variable_name_ is dirty )
                                                     { ... }
                                                      return variable_name_; }

PDoakORNL avatar Dec 20 '19 15:12 PDoakORNL

Is there an area where this is a particular problem? or where you plan to add lots of accessors? (The existing code has many public structs that should have always been classes with accessors.)

One assumption of the suggestion is that variable_name is meaningful outside of its original context. This should be a requirement in either case.

I disagree with the 3rd possibly cached value case. Since at least some of the time it will trigger an expensive computation, we are not really talking about an accessor. I think the function should follow our usual rules ( calcThing(), evaluateValue() ) and simply not be called get. get should be reserved for cheap lookups only. Should the caching become important that could be featured in the name as well, but I suspect we need a wider discussion around how the existence of caches and state machines are indicated.

prckent avatar Dec 20 '19 16:12 prckent

I'm all for disallowing type 3

Well I've made a problem by writing lots of code with the convention

VariableType get_variable_name() const { return variable_name_; }

While I can obviously change my accessors naming without a great deal of trouble...

PDoakORNL avatar Dec 20 '19 21:12 PDoakORNL

I think

VariableType get_variable_name() const { return variable_name_; }

retrun value or const referenece of member

VariableType getVariableName() const { }

for all other cases. @PDoakORNL let us put this rule in the dev doc and close this issue.

ye-luo avatar Apr 28 '22 21:04 ye-luo