Orange light code
Created by: eileenmcnaughton
I just wanted to start a scribble pad for some of the code-patterns currently in the codebase that I think should be discouraged or things I try to discourage in PRs
-
Joins on the option_value table rather than using pseudoconstants - this is highly like to cause an unindexed query so we actively discourage it
-
passing variables by reference - the tide turned on this pattern around 2010 when Xavier pointed out it doesn't save memore due to php lazy loading. The disadvantage of passing & altering a variable compared to returning a variable is that it encourages people to alter the variables to achieve poorly documented, unintuitive or highly fragile outcomes.
-
increasing code complexity without increasing test coverage - increases code fragility
-
silly checks in functions ie checking if one of the parameters in the signature is empty & returning if so - there may be a case for this but often we are just enabling the calling function to do the wrong thing
-
compiled sql. In order of preference for secure concise predictable code we recommend:
- api
- DAO
- CRM_Utils_Select
- compiled sql
- $session = CRM_Core_Session; $userId = $session->get('UserID'); Use CRM_Core_Session->getLoggedInUser()
more to come as I hit them
RE BAOs and apis - it is fine to call a BAO from the api - as long as you do not call the 'same' api - ie. Contribution.Delete api should not be called from BAO_Contribution::delete() as that would be circular. Ditto create.
Create apis call create & add on the BAO, Delete apis call delete() and del() on the BAO (or DAO). Get apis only access the BAO in some of our would-like-to-deprecate apis. The general goal here is to ditch the api v3 get apis that do that