Skip to main content
U.S. flag

An official website of the United States government

Dot gov

The .gov means it’s official.
Federal government websites often end in .gov or .mil. Before sharing sensitive information, make sure you’re on a federal government site.

Https

The site is secure.
The https:// ensures that you are connecting to the official website and that any information you provide is encrypted and transmitted securely.

Code Review

A friendly guide for reviewing code — and not each other — at 18f.

Forked from the excellent Consumer Financial Protection Bureau guide). Help us continually improve by submitting an issue.

Why reviews?

Code reviews are an incredibly important part of the development process. Our goals when reviewing code are to:

Discuss code review with your team

Every individual and team has a different set of expectations around code review. Discussing these expectations explicitly is the best way to make sure that everyone is on the same page.

Document and/or automate the outcomes of your discussion. A common practice is to include this information in a CONTRIBUTING.md file in your project’s repository.

Some guiding questions for your team’s code review discussion:

Tips for a successful review

For a code review to be successful, we all need to be on the same page. To help accomplish this, everyone should consider these tips.

For everyone

For code submitters

For code reviewers

Who merges

There’s lively debate over whether the code author or reviewer should merge the pull request. Follow the idioms that your team has set; if none are present, discuss until you have a consensus. Write that down to resolve the issue quickly in the future.

Opening a PR

We want to receive feedback quickly and make our team members aware of our proposed solutions early, so it’s often a good idea to open a Pull Request for your new feature/bugfix soon after your first commit. We strongly recommend that in-progress work is pushed up at least once a day.

Pull Requests initiate discussion about your commits. Tag your pull request with WIP (either as a Github tag or in the PR title) if you’re not yet ready for a final review. The more information you provide to reviewers in the description, the more context they will have. This leads to faster reviews, and less back and forth between everyone.

CFPB has published a nice PR Template which might make this easier. You can also create a Bookmarklet based on this template.

Submitting code

Before seeking a review, you should be able to check off each of the following:

- [ ] Changes are limited to a single goal (no scope creep)
- [ ] Code can be automatically merged (no conflicts)
- [ ] Code follows the standards laid out in this playbook
- [ ] Passes all existing automated tests
- [ ] New functions include new tests
- [ ] New functions are documented (with a description, list of inputs,
      and expected output)
- [ ] Placeholder code is flagged
- [ ] Visually tested in supported browsers and devices
- [ ] Project documentation has been updated (including the "Unreleased"
      section of the CHANGELOG)

Reviewing code

When reviewing code, you should be able to check off each of the following:

- [ ] Do the changes address the project's needs?
- [ ] Do the changes respect the project's existing style?
- [ ] Does the new code avoid reproducing existing functionality?
- [ ] Are functions/classes as simple as possible?
- [ ] Is the code as efficient as possible?
- [ ] Is the usage of each function/class clear?
- [ ] Have edge cases been considered and tested for?
- [ ] Does the code represent a logical unit of work?
- [ ] Are there any glaring syntax errors that were missed?
- [ ] Are language constructs being utilized properly?
- [ ] Are any frameworks/libraries being used leveraged properly?
- [ ] Are there useful comments/documentation where needed?

Credits