How to review code
Good review practice
You should consider whether PRs:
- have a clear purpose
- capture the simplest way to solve a problem
- suggest changes outside a project’s scope
- need breaking into smaller PRs
You should also consider if a PR’s suggested changes will contribute to technical debt, and if you can make suggestions to help solve existing technical debt.
When you suggest changes, you should explain your reasoning and refer to programming language style guides where appropriate. You may find it useful to provide short examples to explain your suggestions.
- only approve pull requests you fully understand
- give appropriate positive feedback
- flag up major issues quickly, and in person if necessary
- ask for clarification on anything that is not clear
You should not:
- rush a review, even if it’s urgent
- repeatedly point out the same error pattern
- leave comments without context
Good code should adhere to the principles of its language, for example The Zen of Python.
You should consider if the code in a PR has:
- an applicable edge case
- patterns consistent with similar code elsewhere in the codebase
- readable variable names, accurately representing their contents
- missing or additional elements following a merge or rebase
- capacity for reusability
You should always check code for linting issues. You could consider running automated linting before merging PRs, for example with Travis CI.
If a PR involves changes to libraries, you should check that the:
- changes are backwards compatible
- version number has been updated
- changelog has been updated, especially if there are major problems
You should make sure that new or updated third-party dependencies are from reliable and stable sources, and that they do not break any existing code. You should also make sure that they are needed, and that they are open source wherever possible.
You can read more about how to manage third party software dependencies here.
Code changes should have appropriate test coverage. You should consider running tests as part of your review, and check that:
- all possible error cases are covered
- the tests pass in all appropriate environments
- test names describe what’s happening in the test
You should consider if unit tests are enough, or if you need integration tests.
You should consider whether code changes will impact the deployment process, and check that they:
- are self-contained
- do not adversely affect other applications and systems
- do not have any potential security issues
- will deploy properly
The Service Manual contains some guidance about deploying software.
Before you approve a PR, you should consider if it affects any existing documentation. If the PR itself contains documentation, you should make sure that it’s clear and unambiguous, and in the right place.
If you want to learn more about writing clearly for technical audiences you can contact GDS technical writers using the #ask-technical-writers Slack channel.
You can find out more about how to review code by reading: