Doing the dishes (AKA code cleanup)

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.

Code Cleanup

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.

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

API Layer

civicrm_api3

(no try catch required)

Form Layer

try {

civicrm_api3(entity, action, params)

}

catch (API_Exception $e) {

// handle exception

}

test layer

$this->callAPISuccess

OR

$this->callAPIFailure

OR

$this->callAndDocumentAPI

BAO Layer

civicrm_api3

(try/catch is optional as BAO calls should always be wrapped in form or api try catches)

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

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

Form Layer

In static functions

throw new CRM_Core_Exception

Otherwise

Consider replacing with

CRM_Core_Error::statusBounce

if the person is not truly being redirected.

BAO

throw new CRM_Core_Exception

if the redirect is required and you don't know how to move it to the form layer you can pass the redirect information to the form layer as a temporary improvement

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 :-)

Filed under

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.

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).