===============
The most effective way to get bugs out of code is code review. More than running the code, more than unit tests, having someone else review an author's code is the best technique known to eliminate bugs (Fagan 1975 and Cohen 2006).
In general, the later in a lifecycle you discover a bug, the more expensive it is. Meaning we should do what we can to root out bugs at the earliest level possible!
But almost as important is that by having a code review, then at least one additional person becomes familiar with the new code. It acts as a form of knowledge transfer.
Before any Pull Request (PR) can be deployed or merged, it must go through a review process. Depending on the nature of the code, this will include a combination of:
- Developer reviews
- Design review
- QE review
Reviews must be done by someone other than the author of the PR. The main point is to ensure that more than one set of eyes have looked at the code to help verify quality and correctness.
Reviewers are encouraged to leave comments and ask questions in the PR. If the code in the PR is not easily understandable, or something seems incorrect, this should be explicitly called out. Reviews are often iterative, back-and-forth processes, with questions and answers going back and forth between the author and reviewers.
Whenever a new PR is created, Github will automatically insert the following at the top of the PR
### LGTMs
- [ ] Zoom LGTM
- [ ] Dev LGTM
- [ ] Design LGTM
- [ ] QE LGTM
As individual reviewers sign-off on the PR, they will check the appropriate box, as well as leave a "LGTM" comment ("Looks Good To Me") in the PR itself.
For a PR that contains no visual/HTML/CSS changes, the author is free to remove the Design LGTM checkbox.
If your PR includes any visual changes, you should include screenshots in the PR itself. This helps give context to reviewers, and lets designers quickly see if what was implemented is per-spec.
Try to give a good description at the top of your PR. Don't force your reviewers to go in blind. Explain what's going on, ensure you have a link to the appropriate JIRA, etc.
Don't open a PR too early. If a PR is opened, it means the author thinks the code is completely ready for review and could theoretically be released if the reviews are successful.
Too many open PRs also clutter the open PRs page and dissuade people from contributing to reviews.
The aspect of this process that will be new to most people is the "Zoom LGTM". Specifically, this is a "live" code review, performed over Zoom, with the PR visible over a screen share, and the author guiding one or more reviewers through their entire PR.
There are a variety of reasons we insist on this kind of review:
- By forcing the author to explain their code to someone else, the author will often find bugs in their own code. We have seen this happen time and time again
- Knowledge transfer. A code review is not just about someone else verifying correctness, but it's also about teaching someone else about the new code. Doing this live is the best way to teach
- When reviewers are given a verbal introduction to the code, they will have better context when later analyzing the code on their own
- Constructive review of a junior developer's code is a great way to mentor them and allow them to jump into the codebase more quickly
Why do we want to teach someone else about the new code? To prevent the problem of only one author having familiarity with a certain part of how the codebase works.
Preferably, the author of a PR will identify one or two people to do the Zoom review with them, and come up with a time on the calendar when the review will happen. We request that an author will attempt to schedule these reviews two-days ahead of time. This gives the reviewers an opportunity to look at the code before the review happens, and it's easier to find reviewers when you give lots of advance notice.
The recommended way to do a Zoom code review is then as follows:
- The author announces they'd like to do a Zoom review, or even better, asks a few particular people to be their reviewers. Preferably, this is done well in advance, to give reviewers a chance to do an initial "pre-review" on their own. Put the scheduled time on the calendars of the reviewers and authors.
- At the time of review, everyone who wants to be part of it will get together in Zoom, and the author will share their screen
- All reviewers will also have the PR open in their own browser, in case they need to leave comments
- The author begins by explaining the purpose of the PR, what the feature/change is, and demoing that feature/change live in their browser
- Once the demo is complete, someone other than the author starts sharing their screen, with the PR itself open
- The author then walks through every line of code in the PR, explaining high-level reasoning behind everything, but with a reviewer controlling the screen share and scrolling
- The author should be sure to point out and emphasize any particularly important/tricky areas of the code
- Ensure that a conversation is happening, with reviewers free to ask questions and make comments
- If the author finds a bug on their own, they should immediately leave a comment in Github, to remind themselves to address the bug later
- If a reviewer finds a bug or wants the code changed, the reviewer should leave a comment in Github. Reviewers are free to ask the author to pause while this is done
Once the session is complete, any of the reviewers are potentially eligible to check the "Zoom LGTM" box in the PR, and leave a "LGTM" comment in the PR. However, just being present for a Zoom session is not alone enough to sign-off.
Instead, after the Zoom walk through, the reviewers must take a final look through the PR on their own, as if they were doing a "regular" code review.
The reason for this is that with all the extra context provided by a Zoom review, the reviewers are in an even better place to understand the full scope of the changes, find bugs, and suggest changes.
When going through the code, we prefer that one of the reviewers controls the screen share, not the author.
This eliminates the possibility of the author quickly skimming over/scrolling-through areas of the PR. The reviewer gets complete control as to when to "move forward" in the review, ensuring that they actually understand what the author is explaining to them.
This is the more "traditional" code review. A reviewer goes through the entire PR on their own, looking for bugs, leaving comments to ask for clarity, etc. It has the same overall goal as a Zoom review (checking code and knowledge transfer), but is done completely on the reviewer's own time.
If a reviewer is happy with the code, and no other people currently have open questions/concerns/comments on the PR, then the developer can check the "Dev LGTM" box and leave a "LGTM" comment in the PR.
If your PR has any visual changes/features, then one of our designers will have to give their sign-off. Their sign-off is also used to ensure that the visual changes properly reflect any mockups that might be associated with the story.
If your code adds or modifies anything to do with our tests, then QE sign-off is required.
And someone from QE must try the code in a live environment before you're allowed to deploy to production.
All PR authors must undergo the regular review procedure. No one gets special treatment, we all must have our code reviewed in the same way. Remember, a major reason for code review is knowledge transfer. If a senior dev is not being thoroughly reviewed, it means no one else is learning about the code.
If a reviewer is able to find bugs in a PR, that's great! If a reviewer has no idea what's going on in a PR, then they are also encouraged to ask questions and have things explained. Code reviews are about knowledge transfer! We want as many people as possible to be familiar with different areas of the code base.
If a PR has been marked as reviewed but only has "LGTM" comments on it, everyone should be wary. Experience has shown that almost every PR of decent size/complexity will generate at least one comment.
Overly large PRs are problematic for everyone. They're painful for the author to have to explain line-by-line, and they're painful for reviewers to try to understand.
As much as possible, try to keep your PRs small, splitting functionality across multiple PRs if necessary.
We encourage having more than one reviewer involved in a Zoom code review session. This increases the breadth of the knowledge transfer, and puts more eyes on the code to help find bugs.
It's especially useful to have experts and non-experts reviewing simultaneously. Experts might have a better chance of finding bugs in a given review, and giving non-experts a chance to see that process will help them grow as developers and reviewers.
If you're working on an area of code that you know someone else has expertise in, then feel free to ask them to be involved in the review. Or for any other reason you might think of, feel free to ask for a specific person.
It's been shown (Cohen 2006) that fatigue sets in after approximately one hour of code review. So please limit the length of sessions!
Any defect found during a code review could be interpreted as criticism of the author. As a reviewer, keep this in mind. Be thoughtful and kind when reviewing others' code.
Just because many defects were found in your code doesn't mean you were sloppy or unskilled. Hard code has more defects. If the codebase is new to you? Then it's all hard code. Difficult code, or code that was difficult for you, is supposed to have more flaws.
Studies have shown that more time spent reviewing equals more defects found. To quote Cohen 2006:
the quantity of defects found in a piece of code has more to do with how much time the reviewer spent looking at the code as opposed to how “good” the code was to begin with.
Difficult/complex code will have more defects in it. We can't avoid this. Thus, reviewers finding more defects is a good thing! We expect defects to be present, and someone to find them. Every case of this is one case where one of our support floor Rackers aren't the ones discovering a bug.
We are all a team. The end goal of a PR is to produce quality code. The author and reviewers work together to meet this goal.
All developers are encouraged to participate as reviewers. We want all of those people to feel comfortable with the code and to feel comfortable acting as a reviewer.
Even if you are new to the project and a senior person is a PR author, please volunteer to help review. It will be a great opportunity to increase your understanding of the project, and it can help the PR author to have "simpler" questions asked. We often take for granted different areas of the codebase, and it's good to get a fresh set of eyes asking questions about those sections.