The following checklist should be applied when conducting a 90% code review.
- 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 - XSS1 and Injection attacks for example.
- Assets should be loaded with a a Protocol Relative URL2
- Fix any broken windows. They are not someone else’s problem. See charter.
- 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.
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)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 i18n3 then check all user strings have been translated.4
- Look out for missing/badly named/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 5
- Repetitive code - DRY 6
- Large classes
- Poor separation of concerns7
- Badly named file/class names
- 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. 8
- Use constants
// Are there enough comments in the code?
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
- 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.
- Tests should make full use of assertions
- Waiting for elements on a page during browser tests is much preferred to
- There should be documentation - README up to date, apiary docs (if applicable), wiki page and/or infra runbook if necessary.
- You should add descriptions to all functions regardless of access, define what they return and if they throw exceptions
- Params should have a type and description
- If in doubt, check out JSDoc9 or phpDocumentor10