In New Zealand workplaces it's common to see a sign in the smoko* room saying "Your mother doesn't live here - do your own dishes".
People tend to follow this instruction as longs as:
a) there is not already a pile of dirty dishes next to the sink
b) and they can see where to wash the dishes, and where to put them afterwards.
c) their colleagues aren't setting a bad example.
If they see a pile of washed cups beside the sink, another pile on a shelf above the sink and a third pile in a cupboard on the other side of the room if might not be obvious to them that the sign writer meant them to notice the new dishwasher recently installed below the sink & that was the correct place to put the dishes (and actually they didn't need to wash them first).
Code maintenance can be bit like that. If you tidy up a chunk of code people untidy it because
a) they are copying & pasting from a part of the code that isn't cleaned up
b) they don't know what the 'right' way to do things is
c) they can see multiple different approaches in play & don't know which is preferred or
d) they aren't aware that there is anything they should even think about doing differently (a good example of this is that windows users aren't aware they are introducing windows-line-endings into the code).
In 4.4 there is a significant amount of code cleanup making it a very developer friendly release (helps address a & c above). Literally thousands of unnecessary lines of code have been removed. This blog is my attempt to improve the sign above the sink (b & d above). If you contribute code please look carefully at the following as there are a number of items in it that were previously considered to be good practice which we are now moving away from. If you review code / approve PRs please help guide code submitters.
| string | Replace with - api | Rationale | Cleanup status | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| eval | proper php code | Do I need to explain? | Proactive clean-up in all php files in 4.4 | ||||||||
|
trailing white spaces (spaces at the end of a line) (in php, sql, tpl & xml & js files) |
no trailing spaces |
trailing spaces don't comply with our coding standard and most people have their editors set to remove them. Whitespace changes in patches lead to hard to read patches. |
Pro-actively eliminated in php files, sql files, tpl files & xml files in CRM, api, sql, templates & xml diretories - a quick grep for ' \R' shows some have been re-introduced \ missed - mostly in the templates directory | ||||||||
|
tabs (in php, sql, js, tpl or xml files) |
two spaces | as per above - we are following the drupal coding standard on this |
Proactive cleanup of code base has removed most of these outside the packages & js directories. (a quick grep found only 1 re-introduced) |
||||||||
|
windows line endings (/r) |
unix line ending | per above | Proactive cleanup although Joomla! directory, CiviCase xml files & iif files not changed as it was so prevalent in these I thought there might be a reason | ||||||||
| civicrm_api |
|
civicrm_api3 is a wrapper for civicrm_api with version = 3 set. The only other difference is that it throws an exception in case of error. If you are making several calls (e.g. from a form or an import job you can encapsulate several api calls, other functions in one api call. The new test functions ensure that every call to the api has the expected outcome and greatly improve debug output where it is not the case (unless the api has called civicrm_api3_create_error - per below) |
Thorough cleanup of API test suite has resulted in increased test coverage for around 2000 less lines of code. Using the new functions revealed a number of bugs & inconsistencies which were fixed. The CRM test suite was not 100% passing so no cleanup was done on it except to prevent regression. No proactive cleanup in other areas |
||||||||
| 'version' => 3 | remove | not required when calling the api with above methods | |||||||||
|
$this->assertAPISuccess $this->assertAPIFailure |
test layer - remove |
less code this->callAPISuccess & $this->callAPIFailure call these implicitly |
Proactive clean up in API suite (none in CRM test suite as tests were not 100%) | ||||||||
| 'in line' __LINE__ | test layer - remove | this used to be useful but for some time now the test suite has returned backtraces | small amount of cleanup- not pro-active | ||||||||
|
trivial failure tests (e.g. checking $params is an array. api enforces mandatory params etc) |
test layer - don't copy & paste these | Most failure tests in the test suite are copy & pastes that add nothing new to the testing. Failure tests should only be added if they test something specific to the api function not the api wrapper. Otherwise the tests belong in the SyntaxConformanceTest class - which tests all entities | no proactive cleanup | ||||||||
| public $_eNoticeCompliant | PHPunit Test layer - remove this line |
The default is now to enforce e-notice compliance on all tests (recently changed). (removing may involve making the code e-notice compliant)
|
Tests that are not e-notice compliant have been marked as such & the test suite will enforce this | ||||||||
| civicrm_api3_verify_mandatory |
API mandatory parameters should be declared in the _spec functions alternative names for fields should be declared in _spec function |
The api wrapper enforces the mandatory fields declared in the _spec functions. In some cases the mandatory params have complex OR requirements - in which case its acceptable to have them in the API But if it's a case of 'financial_type_id' OR 'contribution_type_id' OR 'contribution_type' - in this case the second two should be declared as aliases of the first. Note that fields like 'membership_type_id' accept an id or a valid string type so 'membership_type' can be declared as an alias of membership_type_id if we need to support it for legacy reasons |
ongoing re-active cleanup but still many instances | ||||||||
| civicrm_api3_create_error |
API throw new API_Exception |
throwing API_Exceptions is our new approach & gives better error reporting. It is caught in the api wrapper | cleanup required | ||||||||
| code in api functions | call basic_get, basic_create & basic_delete | Where possible the api functions should simply call the generic functions - obviously there are many exceptions but if none apply then use these | cleanup required | ||||||||
| CRM_Utils_System::redirect |
|
The BAO layer should never do redirects, nor should static form layer functions as the possibility of the function being called from multiple places is implicit. If a CRM_Core_Exception is thrown & not otherwise caught it will be caught by the api or form/page wrapper. The form/page wrapper will perform a status bounce using the message details OR if necessary a legacy_redirect - although it's better to handle that in the specific form if you can find it Status bounce returns the page viewer to the previous page |
some cleanup - more required | ||||||||
| BAO::create($params, &$ids) |
BAO Layer BAO::create($params); (or as an interim) BAO::create($params, $ids = array(); (BAO should also return a BAO or DAO object) |
This was the standard call signature we agreed for $bao->create last year & is what the api can cope with | some cleanup - more required | ||||||||
| BAO::del(anything other than id) |
BAO Layer BAO::del($id) (BAO should also return a BAO or DAO object) |
This was the standard call signature we agreed for $bao->del last year & is what the api can cope with | some cleanup - more required | ||||||||
| complex logic in the form layer or business logic in the form layer |
Move logic to BAO layer or move function to the BAO layer If the logic should always happen when an entity is updated, created or deleted (e.g. inherited memberships should be deleted when the relevant relationship is deleted) then it should go in the relevant crud function. |
The same logic should be available to multiple forms and the api - eg. the functions to send receipts & merge memberships are currently on the form layer but should be accessable by other forms / the api | some cleanup - more required | ||||||||
| Javascript in tpl files | move to the same-named same-place js file. You need to edit the php file to include it & to assign appropriate variables direct to js | some cleanup - more required | |||||||||
| ajax:async | Test & remove | The default of sync is usually better | some cleanup - more required | ||||||||
| commits with no text | Please include the issue number & a brief description in the commits - even if it is same as the ticket title | This makes the commit log much easier to read. |
* it's well over a decade since you were allowed to smoke in the smoko room in NZ :-)
Comments
This is a great format for demonstrating coding styles, thank you for putting this guide together. It might be relevant to also permalink to part of the Civi codebase which is exemplary, to help alleviate the issue of people copy + pasting deprecated snippets from the medieval ages.
Thanks to put it up together.
On the same idea, to make the life of integrators easier:
don't generate html within php, assign all the "potentially useful to customise even if not displayed (eg id)
smarty:Don't put business logic in it, it's only for display, try to use sematic html and put classes and ids
and jquery: wrap everything in a function.
Xavier- I can add those to the table - but would good to have some examples since they are more complex - kind of like what Adam said - maybe links to commits where we refactor bad to good?
Perhaps we could have a competition to get people to provide a patch that implements each of the above as an example (& an improvement). First prize - free accomodation for one week in er... New Zealand (flights not included).