Coding Standards

The following checklist should be applied when conducting a 90% code review.

Security

Code should never contain credentials. These are a property of the environment.
Is the new code opening up a vulnerability? We should all be aware of methods of attack - such as those identified by OWASP¹.
Assets should be loaded with an HTTPS URL wherever possible, or - as a last resort - with protocol-relative URL²

General

Fix any broken windows. They are not someone else’s problem. See charter. However, please be respectful of your colleagues when doing this.
The repository/project should be Continuously Integrated - see Continuous Integration for specifics.
The code should compile and run without error.
No commented out code blocks. Source control has history for a reason.
Look out for spelling mistakes, both in user text and also code itself
Debug statements to the console (e.g. echo, var_dump, System.out.print, console.log) should be removed. Use debug logging instead.
Check for adequate logging. If you were personally supporting this pull request on your own after merging, you should be satisfied with the level of debug.
Similarly check that adequate analytics have been added (where appropriate).
Ensure code is defensive. if(var[1]) has potential to break.
If signatures of functions have changed, check for redundant variables. Also, does the documentation block for the function correctly reflect its intended function and the parameters it accepts and values it returns.
If the project supports i18n³ then check all user strings have been translated.⁴
Look out for missing, badly-named, or misspelt tags.
If introducing a new external dependency, the appropriate version should be locked on to, rather than using fuzzy versioning.
Pick up issues that could/should have been picked up by a linter. If there is no linter, ask for one to be added.
Don’t allow new TODOs without referencing an open issue for discussion.

Code smells ⁵

Repetitive code - DRY ⁶
Large classes
Poor separation of concerns⁷
Badly-named files or classes
Undefined, badly-named, or redundant variables
Unnecessary code where a command might do the trick
Does the code look complicated and is it hard to understand? KISS. ⁸
Use constants
// Are there enough comments in the code?

Performance

We need to be mindful of how our applications perform on many levels. We should look for changes that adversely affect response times, as well as any other negative effects the new code might have. Amongst other things, look for:

Inefficient database queries
Missing indexes on databases
Infinite loops/recursion
Opportunities to break out of a loop early when appropriate
Opening up connections to database/other services and not closing them
Incorrect callback names (as appropriate) - or callbacks triggered too early
Situations where a callback is made but code can continue to execute after this point - with unintentional consequences

Tests

Ensure there are appropriate tests (unit/integration/browser). See testing for more specifics.
Tests that rely on external services can result in failing tests. Use a mock/stub/spy.
Tests should be atomic and pass if they are run singularly or part of the suite. They should never depend on being run in a specific order or rely on the output of another test.
Waiting for elements on a page during browser tests is much preferred to sleep commands.
Do not mock the system under test. Exceptions are only allowed when the system under test gets its external services via getXXX methods (but we should avoid this, preferring dependency injection).

Documentation

There should be living documentation - project READMEs, API documentation (if applicable), wiki pages, and/or infra runbooks. Ensure these are kept up-to-date in code changes.
Docblocks:
- When using a typed language (e.g. Typescript), ensure your functions and methods have sensible names and appropriate parameter and return types. These are your baseline documentation. Only add docblocks if you’re providing additional information, otherwise the docblock is redundant and has the potential to go stale.
- When using an untyped language, always include a docblock. @param and @returns must have documented types. Any tags (e.g. @param or @returns must have an accompanying description.)
- If a function/method can throw an error or exception, it must have a docblock with @throws tags for each scenario.
If in doubt, check out JSDoc⁹ or phpDocumentor¹⁰