Skip to end of metadata
Go to start of metadata

<ac:macro ac:name="note"><ac:parameter ac:name="title">Work in Progress</ac:parameter><ac:rich-text-body>
<p>This page is work in progress. Please discuss and feel free to edit, rephrase, extend with more cases and examples.</p></ac:rich-text-body></ac:macro>

<p>This RFC contains a proposal for additions to <a href="http://framework.zend.com/wiki/x/LIDZAg">current ZF2 Coding Standards</a> regarding method naming. </p>

<p>Conventions of this document:</p>
<ul>
<li><code>ComponentX</code> - component of ZF2, top level class</li>
<li><code>SubcomponentX</code> - subcomponent ComponentX</li>
<li><code>PluginX</code> - loadable plugin for component or subcomponent (Adapter, Reader, Writer etc)</li>
</ul>

<h2>Requirements</h2>
<p>This section proposes standards to be marked as <strong>MUST</strong>, <strong>MUST NOT</strong>, <strong>SHOULD</strong> and <strong>SHOULD NOT</strong> in CS docs.</p>

<h3>General</h3>
<ul>
<li>Public methods should not have underscore. Developers are also encouraged to not have underscore in protected/private methods.</li>
<li>Setters should always return instance to allow chained calls.</li>
</ul>

<h3>Component Configuration</h3>
<p>As a general rule developer should consider using <code>Stdlib\Options</code> for component configuration. Such component/subcomponent should have <code>setOptions()</code> and <code>getOptions()</code> methods:</p>
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
public function setOptions(ComponentXOptions $options = null)
{
if (null === $options)

Unknown macro: { $this->options = new ComponentXOptions(); }

$this->options = $options;
return $this;
}

public function getOptions()

Unknown macro: { return $this->options;}]]></ac}

// Main component class
class Component

Unknown macro: { // creates main component instance public static function create($options); public static function createFromString($string); public static function createFromArray($string); }

]]></ac:plain-text-body></ac:macro>

<p>Another approach:</p>

<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
// Separate Factory Class
class ComponentXFactory
{
public static function factory($options);
public static function subcomponentFactory($options);
public static function pluginFactory($options);
}

// Main component class
class Component
{
public static function factory($options);
public static function fromString($string);
public static function fromArray($array);
}
]]></ac:plain-text-body></ac:macro>

<h3>Singletons</h3>
<p>Methods that control singleton type instances:</p>
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
// setter
public function getInstance();
public function resetInstance();
]]></ac:plain-text-body></ac:macro>

<h3>Controlling Exceptions</h3>
<p>Methods that define object behaviour towards Exceptions (enabling, handling, suppressing, registering etc.) should follow same convention. Proposed names are:</p>
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
// setter
// plural form, even if only one exception is expected
// always accepts flag parameter
public function setThrowExceptions($flag = true);

// accessor
public function getThrowExceptions();

// get list of thrown exceptions
// always returns array, even if only one exception is expected
public function getThrownExceptions();
]]></ac:plain-text-body></ac:macro>

<p>Another approach:</p>
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
// setter
// plural form, even if only one exception is expected
// always accepts flag parameter
public function throwsExceptions($flag = true);

// accessor
public function doesThrowExceptions();
]]></ac:plain-text-body></ac:macro>

<h3>Immutable/Readonly Instances</h3>
<p>Method (and/or configurational parameter) that sets object to readonly/immutable state should have same naming convention. Possible names are:</p>
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
// setters
public function setReadOnly($flag = true);
public function setImmutable($flag = true);

// accessors
public function isReadOnly();
public function isImmutable();
]]></ac:plain-text-body></ac:macro>

<h2>Recommendations</h2>
<p>This sections proposes recommendations to be marked as <strong>MAY</strong> in CS docs.</p>

<h3>General</h3>
<p>Avoid public <code>void</code> methods. If method should not return any value, return true/false/null to signal success/failure of the performed operation. </p>

<h3>Boolean Parameter Setters/Accessors</h3>
<p>There is no way to imply strict rules. Some suggestions may include:</p>
<ul>
<li>setter and it's counterpart accessor should follow same naming convention</li>
<li>if the setter operates with private/protected property, it's name should not be based on property name, because such variable is not exposable through public API.</li>
<li>setter method that starts with the verb that explicitly implies state (enable, disable, allow, deny) should not accept values to prevent ambiguity
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
public function enableLogging($enable = true/false);
public function acceptXml($accept = true/false);
]]></ac:plain-text-body></ac:macro>
Verb "enable" here implies that method enables logging. In this case consider to have separate methods:
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
public function enableLogging();
public function disableLogging();

public function acceptXml();
public function rejectXml();
]]></ac:plain-text-body></ac:macro>
Or use "set" prefix
<ac:macro ac:name="code"><ac:default-parameter>php</ac:default-parameter><ac:plain-text-body><![CDATA[
public function setEnableLogging($enable = true);
public function setAcceptXmlFiles($accept = true);
]]></ac:plain-text-body></ac:macro></li>
</ul>

Labels:
rfc rfc Delete
coding coding Delete
standards standards Delete
methods methods Delete
Enter labels to add to this page:
Please wait 
Looking for a label? Just start typing.
  1. Apr 17, 2012

    <ul>
    <li>I disagree with the rule that having "more than one option" should require an Options class for the given class and/or component. Having 2 or 3 options is fairly normal, and does not clutter the class with too many accessors/mutators. I think this one should be a <em>recommendation</em> only.</li>
    <li>I disagree with flag accessor naming, as noted on the ML; "is" and "has" are only two of the ways you can ask a boolean question. Forcing usage of one or the other of these will lead to phrases that are unreadable in normal language and/or do not properly convey intent.</li>
    <li>The "suppressing exceptions" section is inconsistent with other flags and options. The setters should follow the same paradigm as any other setter, the accessor the same paradigms as boolean accessors (throwsExceptions() would be proper English here).</li>
    <li>The very, very few cases in ZF/ZF2 where we have immutable objects do not justify a rule at this time.</li>
    </ul>

    <p>Basically, at best, I see these as <em>suggestions</em> or <em>recommendations</em> that are reasonable to establish as conventions, but not as <em>rules</em> we need to codify.</p>

    1. Apr 17, 2012

      <p>Fair enought, let it be recommendations not rules.<br />
      Then question, if someone during beta stage submits a pull request with component refactoring towards these <span style="text-decoration: line-through;">standards</span> recommendations (with tests and docs of course) should it be merged?</p>

    2. May 03, 2012

      <p>I think we NEED rules. SOME rules, maybe not exactly this rules. But consistency in cornerstone of ZF. </p>

      <p>And a small rant - "proper English" would be "doesThrowException()" wouldn't it? As in:</p>
      <ul class="alternate">
      <li>Is it enabled? -> isEnabled()</li>
      <li>Does it throw exceptions? -> doesThrowExceptions()</li>
      </ul>

      <p>I also think (as I stated on ML) that is/has should be used whenever possible. Many directives can be renamed so that is/has makes sense. As enableLogging - it can be renamed to loggingEnabled to allow isLoggingEnabled and setLoggingEnabled. It does not work for every case this good (as for slightly crappy isXmlAllowed / setXmlAllowed), but still it should be the encouraged way of naming IMO. </p>