Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Rename accessor names. #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Thinkscape
Copy link

This is not BC by a long shot, but I believe the current interface names are awkward.

The change

  • instead of function dimensions() it should be function getDimensions()
  • instead of function options() it should be function getOptions()

Reasoning

Dimensions and options are values on the config (value) object, hence retrieving them via methods is accessing; those methods are called accessors. A common practice is for accessors to be aptly named to recognize their function by prefixing get or set accordingly.

Assuming the (config) class contains multiple action methods such as merge() performAction() or even setOptions() (i.e. a mutable config) then the interop interface should hint getters with names that describe their purpose: getOptions() and getDimensions(). This makes them less ambiguous and more in line with other libraries and practices.

@sandrokeil
Copy link
Owner

Thanks for the suggestions and you are right, but this library uses a Domain-Driven Design approach and the factories should be immutable. I think in most cases, the factories are created by the service locator. During the planning phase we came to the conclusion to avoid the get prefix and the Interface suffix.

@Thinkscape
Copy link
Author

Sadly I wasn't involved in your planning phase, but I'd really like to understand it.

Could you elaborate on that?

Do you regard those factories as Domain Logic objects? It would go with the rule that the construction should be solely in constructors and accessors should be avoided. I know the limitations of PHP and I predict that you went with getters, instead of just properties, for flexibility; however that doesn't explain why you'd name your getters ambiguously and drop "get" prefix. ?

@sandrokeil
Copy link
Owner

I would not say that the factories are domain logic objects. It's not intended that the factory has setters, because it makes no sense to have a dynamic config structure for a factory. You don't have properties for these values too. Why should I have a get method (getDimensions) when I have no setter or it does not rely on a property? Of course you can, but normally you would not.

It's more a naming design decision. It's like should I use the interface suffix or not? In the first version, there were get methods. But it is unnecessary. Some people have prefixed the getter methods of value objects with get, but why? There are no setter methods with the corresponding name. There are only named constructor methods.

@Thinkscape
Copy link
Author

I supposed the answer to why those are not just $properties is that you wanted to constitute a php interface.

Ok, so you wanted to have methods. getProperty explains the intent and purpose of the method.

Example: consider a (factory) class that has multiple methods:

class whatever {
  public function __construct(){}
  public function validate(){}
  public function load(){}
}

Each method has a purpose, including the PHP magic method of __construct(). All of those explain the purpose and the action being performed.

Now let's add some accessors:

class whatever {
//public function __construct(){}
//public function validate(){}
//public function load(){}

  public function getVersion(){}
  public function getTime(){}
  public function setMode($mode){}
  public function resetTime(){}
}

Accessors have self-explanatory name which is somewhat programming language-agnostic and is a general pattern of how you name accessors. It does not matter how many you have, if they're symmetrical for each piece of data, or if they are all setters, getters or other mutators.

Even if I removed all mutators, I end up with:

class whatever {
  public function __construct(){} // ... constructs
  public function validate(){}    // ... validates
  public function load(){}        // ... loads
  public function getVersion(){}  // ... gets version
  public function getTime(){}     // ... gets time
}

The reason for methods to have names it to explain purpose and possible effect. Methods are callable, hence it is a good practice to name them using verbs describing the action. Accessors follow a well-known naming convention (get* set*) to make them easily recognizable, at a glance. The words dimensions and options do not explain the type of accessor nor hint that it is supposed to be an accessor in the first place. Because they belong in an interface, lack the body, that makes them even more ambiguous and confusing until someone reads into the method description in docblock.

The only counterargument I found is the normalization of methods with a well known relation - that's why i.e. some SPL classes would truncate accessor name if the relation is static and well defined, so the class cannot operate on anything else but the same type of items. The problem I have in this case is, that RequiresConfig interface forces the implementing class to become one-purpose and follow flattened naming (or refuse to contain any other methods at all).

@Thinkscape
Copy link
Author

Just teasing, but why is this
public function canRetrieveOptions($config);
... not called ...
public function retrieveOptions($config);

I know it changes the meaning, but it seems that this method is named purposefully compared to the other two.

Some people have prefixed the getter methods of value objects with get, but why?

Factories are not value objects. But even if they were, this prefix makes sense for consistency and readability.

@sandrokeil
Copy link
Owner

In my PoV interop-container factories doesn't have setters and are stateless at default. Not using the get prefix doesn't mean not to use descriptive names. Let us see if someone else has some suggestions.

@Mairu
Copy link

Mairu commented May 31, 2016

I am with @Thinkscape on this matter.
I don't see why the get prefix should indicate that an object is not stateless or you retrieve a property with that method.
Best example was already mentioned with canRetrieveOptions vs options. If you start naming your methods without describing verbs you get into trouble (sticking to the same naming pattern) when adding methods for the same noun.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants