Class method naming conventions

[ghost]

I think we could agree we've had troubles previously with class method naming conventions which have caused a few heartaches. Two issues relating to this have been introducing bugs due to methods behaving unexpectedly (i.e. a get method will perform more than just returning a value), and our code not being refactored totally correctly. (i.e. a lot of reused code in those costimator importers which perhaps could have been refactored given better 'bite size' coding).

Just thinking about it quickly tonight, could we start thinking about building a convention such as Request-Modifier-Element for our methods.

Request

Somewhat in the same vein as HTTP with safe calls as well. Just some ideas to start:

• getSomeValue() get methods are classified as safe, in other words they return only the element asked for, they don't affect the state of the entity in any way.
• fetchSomeValue() fetch methods behave the same as get but they may affect the entity in some way such as loading a cache etc. These methods have no logic in them though, they only call get has and perform methods.
• hasSomeValue() has method are also classified as safe, returning a boolean element.
• isSomeState() is methods non-safe boolean methods, but like fetch only call get has and perform methods.
• performSomeAction() perform methods contain actual logic relating to the class, and are the only methods which do. I'm thinking there might be a few more good conventions here somewhere with these methods being private, or perhaps unable to return values... In the case of entities, these methods should exist and be held within the manager.
• setSomeValue() set Much the same as we have now, but are safe in the fact they only affect one property in the class.
• splitSomeValue() split Somewhat the same as set but may affect numerous properties. In effect, splitting the given variable in certain ways.

  Modifier

This (optional) verb describes what the method actually does. As long as this modifier is 'verby' then I don't think there is much more we can do here. Safe methods most likely won't use modifiers at all, but they will be needed most of the time on the non-safe methods.

Element

I've called this element and not property based on the idea that it might be an abstract part of the entity. For instance, within the pager we might have ResultsPage, TotalResults or PageNumber. PageNumber would be the only actual property of these three.

More points to ponder

• We'd need to figure out whether which order of RequestModifierElement or RequestElementModifier will work in most cases. I think switching between the two is a bad idea, while sticking with one will mean cases of methods such as fetchResultsPageNext or fetchCountTotalResults. My 'feeling' is modifier first will lead to better semantics.
• Manager classes (or any modifier class) will need some thought. I think the same ideals of having public/api/interface methods and private/performance/logic methods will be beneficial.

Still pondering...

[ghost]

@leevigraham @iainsaxon

[leevigraham]

See: http://symfony.com/doc/current/contributing/code/conventions.html#method-names

[leevigraham]

Not sure about getSomeValue() and fetchSomeValue() being two different things. Exposing a getSomeValue() is probably enough. I understand that fetch could load some things into the entity or a cache but as someone implementing the class and calling the methods I don't really care about that *as long as the getSomeValue() is still safe.

Can you provide an example of splitSomeValue()?

RequestModifierElement or RequestElementModifier

I think this should be verb+Element+Modifier. Example: getOrderTotal() getOrderSubTotal(). It reads better to me. The other thing to consider here is count, sum, avg and other aggregate modifiers. These should also be the final prefix IMO.

So you could have:

[get|set|has|is|perform|replace|remove|all|add|register|count]
+[Property|EntityName]
+[Sum|Min|Max|Count]`

Actually looking at the line above aggregate values must only be used with get prefixes.

[leevigraham]

Additionally naming conventions for aggregate fields stored in the DB could reflect their collection.

Example instead of getOrderTotal() we could use getOrderLineItemCostSum()

I think we should never use getSomethingTotal. Is it total number of items in a collection or a total of one of their properties. Sum and count are much better. Lots of people smarter than us figured this out a long time ago :)

[iainsaxon]

If we are moving towards the idea of simple entity-value containers where the entities do not do work or change state then the fetch* method prefix may not be needed. The entities themselves will not alter their own state because a service/factory/manager has set the values.

I guess this is headed towards a system where side-effects are limited as calling get* method on the entity won't trigger state change that may be unexpected. The entities that are sent to the views/templates remain the same in the controller and the views.

This could also mean that any tax-related functions would have to be sorted out in the controller before the item is sent to the view. While debugging code would be a lot easier as you would only need to look in the controllers and managers for entity state-changes the view side could get a little trickier as returning getPriceIncludingTax would only work if the tax was calculated before the entity was sent to render.