graze / standards

⚖ The coding standards of the graze tech team.
https://graze.github.io/standards/
MIT License
6 stars 2 forks source link

Boolean method naming standard #27

Closed wpillar closed 8 years ago

wpillar commented 8 years ago

This is my personal preference and it makes a lot of sense to me. I like that this kind of convention enables more self-documenting code, I see is*() or has*() and I know it will return me a bool. Also makes it easier to find existing methods and not duplicate them.

The example that triggered this:

appliedCardRefreshReward() vs hasAppliedCardRefreshReward()

Please vote 👍 or 👎 using the reactions.

john-n-smith commented 8 years ago

I'm down with this.

How would you rename supportsMaterialVersion()?

We might need can*() too. ...or we add 'able' to words, interface style.

$this->canSupportMaterialVersion() $this->isMaterialVersionable()??

wpillar commented 8 years ago

Updated to provide a general recommendation rather than a strict rule.

mathieupinet commented 8 years ago

I grep'ed our codebase to few counter-examples to explain why it could be a difficult guideline to enforce :

Sometimes booleans are used as a measure of success of the performed action

In some cases, the only way to enforce that rule is to use the present progressive or another weird: *Schedule_Recurring::occursOnDate() (could be isOccuringOnDate, which is worse grammar imo)

Out of the 100+ @return bool methods that I looked into, 90% were already expressed as is* or has*, and only a couple could have been rephrased. @wpillar what called for you to write this rule?

wpillar commented 8 years ago

I'll jot thoughts next to your examples that I hope explain my general thinking:

Profiler_RuleSet::toCache() Graze\Admin\Production::ProductProductionLinePrinterTemplateMapSaver::save()

These ones are odd, yes they return a bool but they also change the state of something, using bool as success feedback. These aren't the kinds of methods I was thinking about, but I can see how the way it was written could imply that.

Promotion_Code::codeExists

Fair. Can't think of a better name really, which is why the recommendation isn't a strict rule.

Promotion_Phase::showLimitedOffer()

This is a bad method name IMHO. Does this method do the showing of the offer or tell me if the offer should be shown? showLimitedOffer() should be code that does the showing, canShowLimitedOffer() or shouldShowLimitedOffer() is more precise and would determine if the offer should be shown. One example that I think shows the need for some guidance.

Graze\Web\Product\Nutrition\HealthWarning::exceedsThreshold()

Again fair, a good descriptive name already, the recommendation probably wouldn't apply here.

biggianteye commented 8 years ago

👍 to latest incarnation mentioning auxiliary verbs.

mathieupinet commented 8 years ago

@wpillar I agree on showLimitedOffer(). All I'm pointing out is that it's far from applicable to all boolean-returning methods; but if people think common sense will prevail and it's helpful as a reference/guidance for PRs, then 👍 .