How to review code
Comprehensive reviews help to keep code maintainable and reusable, and reveal gaps in documentation. Use the GitHub review feature with pull requests (PRs) before merging code. The author is responsible for obtaining a code review, and merging the pull request.
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. See Programming language style guides for more information.
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:
- 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.
Find out more about writing and reviewing pull requests on the GDS Tech Learning Pathway.