configuration getters - method name and intent consistency

[fooman]

This reopens #443

Throughout the code base there are a few different approaches to naming methods when retrieving a configuration setting. From app/code/Magento/Tax/Helper/Data.php

/**
 * Get configuration setting "Apply Discount On Prices Including Tax" value
 *
 * @param  null|string|bool|int|Store $store
 * @return bool
 */
public function discountTax($store = null)
{
    return $this->_config->discountTax($store);
}

/**
 * Get value of "Apply Tax On" custom/original price configuration settings
 *
 * @param  null|string|bool|int|Store $store
 * @return string|null
 */
public function getTaxBasedOn($store = null)
{
    return $this->_scopeConfig->getValue(
        Config::CONFIG_XML_PATH_BASED_ON,
        \Magento\Store\Model\ScopeInterface::SCOPE_STORE,
        $store
    );
}

/**
 * Check if tax can be applied to custom price
 *
 * @param  null|string|bool|int|Store $store
 * @return bool
 */
public function applyTaxOnCustomPrice($store = null)
{
    return (int) $this->_scopeConfig->getValue(
        Config::CONFIG_XML_PATH_APPLY_ON,
        \Magento\Store\Model\ScopeInterface::SCOPE_STORE,
        $store
    ) == 0;
}

Some other examples:
app/code/Magento/Backend/Block/Page/System/Config/Robots/Reset.php

public function getRobotsDefaultCustomInstructions()
{
    return trim((string)$this->_scopeConfig->getValue(
        self::XML_PATH_ROBOTS_DEFAULT_CUSTOM_INSTRUCTIONS, \Magento\Framework\App\ScopeInterface::SCOPE_DEFAULT
    ));
}

app/code/Magento/Backend/Block/Page/Notices.php

public function displayDemoNotice()
{
    return $this->_scopeConfig->getValue('design/head/demonotice', \Magento\Store\Model\ScopeInterface::SCOPE_STORE);
}

app/code/Magento/CatalogInventory/Model/Resource/Indexer/Stock/DefaultStock.php

protected function _isManageStock()
{
    return $this->_scopeConfig->isSetFlag(
        \Magento\CatalogInventory\Model\Configuration::XML_PATH_MANAGE_STOCK,
        \Magento\Store\Model\ScopeInterface::SCOPE_STORE
    );
}

It will be a lot easier to work with Magento if one approach is chosen and consistently used.

[tang-yu]

For a method that returns a non boolean value, the method name should start with "get". For method that returns a boolean value, the general convention is to name the method as if a question is asked. For example, "discountTax" can be interpreted as "shall we discount tax?". However, people ask questions in many ways and that's where the inconsistencies come in. The same question can be asked as "is tax discounted?" which would translated to method name "isTaxDiscounted". In this particular case, I think "isTaxDiscounted" and "isTaxAppliedOnCustomPrice" are better method names.

There are thousands of methods that return boolean value in Magento, it would be a big undertaking to review all of them. We may adopt a consistent naming convention and gradually refactor existing non-compliant methods.

[Vinai]

I think it would be valuable to have a standard naming convention for methods returning booleans in place.
+1 from me for starting such methods with is or has, which happens to be a very common convention thanks to Uncle Bob suggesting that in Clean Code.
And of course you are right, refactoring can happen over the coming years, but that is no reason not to have a standard.

[alankent]

We should review implications with web service deserialization code as well for data entity interfaces. This may be more than just a coding convention - it may be baked into some areas of the code.

[fooman]

@tang-yu thanks for the extra details. I am hoping that a standard can be agreed upon on soon (using is or has sounds great). We have a window of opportunity to do the refactoring during the developer beta. I disagree that this can be worked on easily later down the track since you will have to worry about backwards compatibility so you would end up with:

public function applyTaxOnCustomPrice($store = null)
{
    return $this->isTaxAppliedOnCustomPrice($store);
}

public function isTaxAppliedOnCustomPrice($store = null)
{
    return (bool) $this->_scopeConfig->getValue(
        Config::CONFIG_XML_PATH_APPLY_ON,
        \Magento\Store\Model\ScopeInterface::SCOPE_STORE,
        $store
    ) == 0;
}

[TexanHogman]

@fooman for Magento2 we are claiming our coding standard to be PSR-1, PSR-2. So best practice, yes but enforcing as a standard probably not.

In fact internally we have a statement
Methods that return certain status flags or other boolean values, must not start with "get", but should be prefixed with "has" or "is".
but again seen as a best practice versus something enforceable.

We'll open a ticket to look at these changes to honor our own best practices. PRs welcomed too.

[magento-engcom-team]

@fooman Thank you for the suggestion. Please refer to the Community Forums or the Magento Stack Exchange site for advice or general discussion about this issue. The GitHub issue tracker is intended for Magento Core technical issues only.