Code Review

To make it easier for us all to work together we need to ensure that we are working to a common set of standards, and that the way we are coding is as easy to follow as possible.

1. The projects coding standards are being met.

2. The code should be easy to understand, comments can and should be used to assist with this.

3. PHP docs have been updated and help with the understanding of the code

1. The commits are split logically (i.e. a single short line should be able to summarise the change in each commit)

2. Each commit message starts with a ticket number followed by a short 1 line summary of the change

3. Where changes are complicated there is a blank line followed by longer description

1. PHPdocs and comments are useful (Make sure that value is added, i.e. the function name is not just repeated)

2. Significant changes have had the appropriate specifications and developer documentation updated

3. Commit messages help explain changes

1. Where possible output is not mixed in with programme logic (this makes things easier to maintain) some projects will make this easier than others.

2. Output conforms to the project HTML standards.

3. Inline styles should not be used (external stylesheet files are better for maintainability)

1. Output should be in a form that can be localised

2. Language used will be understandable by end users

1. Minimal database calls (i.e. calls in loops are generally bad for performance)

2. SQL is written in a way that will work on all MySQL database engines supported by the project: innodb/ndb (and MyISAM for help tables)

3. User input is not directly added to SQL without any form of cleaning

1. Appropriate logins and capability checks are performed

1. No unnecessary personal data is collected

2. Personal data is stored and used in a way that is compliant with the General Data Protection Regulation (GDPR)

1. Database queries look efficient.

2. Expensive code is not on a critical path in the system and is not used on every page load (i.e would cause a page to be slow)

3. There is not excessive looping

1. Does the code look like it solves the problem it set out to do, without creating additional side effects (i.e. it does not make changes outside of the scope of the ticket)

2. The code makes sense when considering the whole system (for example if a single line in a method was changes look at the whole method to understand how it fits in)

3. Are there other parts of the code base that could have been affected by the issue?

4. The change is not mixing a re-factor and functional change (this is much more complicated to review)

1. That appropriate unit and functional tests have been added

2. That there are notes on the ticket to help the test team determine what the scope of testing should be