This article explains why code reviews are essential for growing a 10x software engineering team. The first part of the article is theoretical, while the remainder uses this theory to make practical suggestions about using GitHub pull requests for code reviews. Please note that at Innolitics we write software for medical devices. Thus, although most of my suggestions are general, some may not apply to your situation.
Why do we do code reviews? 🔗
Why do we do code reviews? A high-level answer is “to provide value to our users and other stakeholders.” At Innolitics, we might say: “to mitigate safety risks, help patients, and make our clients profitable.”
Here are some lower-level answers:
We review code to catch bugs. That is, the author of the code meant it to act one way but actually acts another way. The reviewer might notice this and then it can be fixed.
We also review code to improve readability. Even if the code works, it can be difficult to understand. For example, the author may have used generic function names or their functions are very long. Code tends to make more sense to the author since they wrote it, so a reviewer can more easily spot poor organization or ambiguous names.
Good reviewers also check that the new code is consistent with the existing code’s architectural design. Reviewing design is more difficult and time-consuming since it requires a deeper understanding of the project.
It’s also good to check for tests. Are there enough of them? Do they test the right things?
You may want to check that all the stated requirements were met. It is easy to forget this step.
Your project may have other specific things to check for. For example, you may check that the documentation has been updated or that new risks have been documented and or mitigated.
Thus, we do code reviews for several reasons:
- Improve correctness (catch bugs)
- Improve readability
- Improve the design
- Improve testing
- Improve documentation
- Check that stated requirements were met
While there are many positives to performing code reviews, some negatives also exist:
- Code reviews oftentimes require the reviewer to perform a significant context-switch, because they must go from working on their own activities to reviewing the activities of another. Context-switches can result in a few minutes of inefficient work.
- While waiting for a code review to finish, the author might be blocked and unable to work on developing new features or fixes.
- Code reviews, when not given a healthy sum of effort by team members, can inspire false confidence in the level of security provided from the code reviews.
- Without a process in place for keeping engineers focused on the right issues for the given time, authors and reviewers may debate the importance of fixing certain bugs or introducing certain features, which results in lost time and confusion amongst team members.
While these negatives are defeated by the sheer benefits of performing code reviews, they must be recognized; improving how you conduct code reviews can and should reduce the amount of effect these negatives have, and should improve the team.
Good code reviews improve the team 🔗
Good code reviews improve the team, and improving the team is the most efficient way to improve the code in the long run.
Why is this true? The short answer is that small improvements quickly compound. It’s the same logic that justifies investing in your development environment—incremental improvements in productivity pay for themselves quickly.
Someone may say: “I agree that if you can improve yourself one percent each day, you will become a 10x coder in no time! But are code reviews the most efficient way to improve yourself? Why not read Code Complete or watch online lectures instead?” We should use these resources too, but they tend to provide generic or theoretical advice. Code reviews give us concrete, nuanced situations to discuss—not the toy situations that books use due to lack of space and attention. Another benefit of code reviews is that a good reviewer will tailor their feedback to you.
Also, some improvements are project-specific, and won’t be found in books. In the least, code reviews familiarize team-members with the codebase. But deeper team-improvements can also occur. For example, the reviewer and the author may realize they’ve both been struggling with the same design problem, and then, through reasoned discourse, they come to a solution that neither was able to find on their own. Grace and I had a moment like this just the other week.
I don’t think code-reviews will automatically improve the team. It’s not a silver bullet. Code reviews are a lot of work. And to be effective the team must value learning and improvement and have mutual respect for each other.
How code reviews improve the team 🔗
The observation that code-reviews improve the team has several implications. The most important is that in-depth code reviews are worth the time if the author and reviewer are improving themselves.
Without this realization, it may feel like time misspent to discuss whether a linebreak should be placed between a set of statements, or to debate two variations of a function name. However, the time investment can pay for itself and more during the rest of the project.
One of the reasons this is possible is because, if the author is improving, the length of the code reviews will decrease with time.
In this chart, there is a skill-gap between the reviewer and the author. With each review, the skill gap shortens and so too does the review duration. I’ve drawn one larger review on the right. This is meant to represent an in-depth discussion that lead to a realization, thus bringing both the author and reviewer to the next level.
There are yet a few more ways that code reviews improve the team. In addition to seniors teaching juniors and equals teaching each other, juniors can also learn by reviewing a senior’s code and asking questions. If you think the main purpose of reviewing code is to improve the code, then a junior engineer reviewing their senior may not feel the need to do in-depth reviews. This is not the case. You should ask questions about the code you don’t understand.
Finally, I often learn a lot by reviewing junior’s code. Often I feel that a particular approach is off, but I can’t articulate why. Then, in the process of articulating my thoughts, I can better understand my thought process (and, sometimes, realize I’m mistaken or over-generalizing).
- Seniors teach juniors with reviews
- Equals teach other
- Juniors learn when reviewing
- Seniors learn by teaching
Thus, in conclusion, in-depth code reviews are a great way to improve the team, and thus the code.
Practical suggestions 🔗
GitHub pull requests 🔗
There are many ways to review code. E.g., you can email people a code-diff or you can do an over-the-shoulder review. At Innolitics, we usually use GitHub’s Pull Requests, so this article will focus on how to use them effectively, but most of these tips will apply to GitLab, BitBucket, and other git hosting platforms too.
When should you create a PR? 🔗
The first question an author needs to ask themselves is: When should I create the PR? There are several options:
- After construction and testing
- During construction
- After the design
If you’re uncertain about your design, request a design review. Once all the code is constructed, changing the design can be expensive so the extra review is usually worthwhile.
Junior engineers should be especially quick to request design reviews. Also, project leads should suggest design reviews in the issue description if you think the design may be unclear.
How big should your PR be? 🔗
The next logical question is what should be included in a PR. Here are a few pointers:
- Smaller PRs are easier to review
- It is possible for PRs to be too small
- Cleaning commit history may make bigger PRs easier to review
Smaller PRs tend to be better. E.g., if you spend more than two weeks coding without a review, you should probably create a PR. Too many small PRs would be annoying, although I’ve only seen this happen a couple of times. People tend to err towards too big.
If you have an especially large PR and your reviewer is busier than you are, you can save them some time by squashing all the changes and creating a new set of logical commits. In essence, this creates a sequence of smaller PRs for the reviewer at the cost of your time and possibly some development history.
Who should you assign to the PR? 🔗
This is more of an issue for larger teams, since on a small team it will probably be obvious who should review the code.
Senior engineers should review modifications that are higher risk. For example, on medical device projects built following the IEC 62304 standard, the software system is decomposed into smaller parts and each part is given a safety classification of A, B, or C. Senior engineers should review the highest risk parts.
Consider using a CODEOWNERS file. This file indicates who should review specific files. It can be combined with protected branches to ensure particular people review PRs that touch particular files. This is especially helpful if you want to be sure a particular senior engineer always reviews modifications to a high-risk part of the system.
Since code reviews level-up the team, consider assigning reviewers who will learn the most.
You may want to assign multiple reviewers if one of them is junior or there is high-risk code.
What should you put in the opening comment? 🔗
When you create a pull request in GitHub, you must provide an opening comment.
What should you write there?
- NOT information that will be useful after the merge
- What to focus on
- File review order
- Suggest commit-by-commit vs. full-diff
- Screenshots, if UI changes
If comments will be useful long after the PR review, considering adding them to the codebase directly instead; answers to “why” questions or alternatives may make more sense in commit messages or comments.
PR comments are harder to track down than git commit messages or changes in the code. Also, comments in PRs are less attached to the code than comments or documentation. E.g., we’ve lost GitHub comments a few times in the past when changing repository hosts or merging repositories.
Note: It can also be useful to write comments in your own code. E.g., to point someone to some code you’re worried about.
There are some exceptions to the guidance discussed above. For example, on some of our projects we use a PR template to provide a standardized list of regulatory-related questions about the changes made in the PR. This information is pulled out of GitHub using our regulatory documentation manager and inserted into the appropriate regulatory records.
When should you push changes to an open PR? 🔗
If you push new commits to a branch that has an open PR, those commits will show up in the branch. This can be frustrating to the reviewer if they’ve already begun their review and you then change out the code from under them; they may need to go back and rereview your code or edit their now-stale comments. Still, there are situations when this is okay:
- If you are only adding comments or more documentation
- If you know the reviewer hasn’t started the review yet
- If you notice a bug (and pushing a fix will save reviewer’s time)
Don’t push code if the reviewer has started and changes can go in the next PR.
When should you review? 🔗
As the reviewer, when should you start reviewing a PR? Don’t start until you’ve been assigned as a reviewer, since until then you don’t know if the author is ready for you to look at the code and you may waste time looking at something out-dated.
Once you are assigned, if you can begin reviewing immediately you may save the author the need to context-switch to another task. This comes at the cost of the reviewer needing to context switch. Thus, consider whether it is more costly for the reviewer or the author to context switch. E.g., if you as the reviewer are just getting back from lunch, your cost of context switching is low.
Even if you don’t have time to do the full review, even a few quick comments can save the author a context switch if it gives them enough to work on until you can finish the review.
Of course, also consider if the author has other things to review or if the review is blocking other people on the team.
Should you batch comments 🔗
GitHub let’s you send comments in batches or one-by-one.
If you begin reviewing the PR immediately after it is created, it’s often better to send comments one-by-one so the author can respond while you’re reviewing, thus avoiding a context switch. Be sure to tell the author if you start the review so they can take advantage of this. Also, you may be able to get through a couple rounds of back and forth responses on your earlier comments before you are done with the entire review.
If, on the other hand, you know the author has moved on to something, then use a review to batch your comments together. This avoids needlessly interrupting the author with spaced-out notifications. Batching comments will also increase the likelihood that you can answer your own questions. E.g., often if I’m reviewing commit-by-commit, sometimes a later commit will clarify a question I had about an earlier commit. If you send the comments to the author one-by-one, they will have to sift through your out-dated comments.
How long should you spend on the review? 🔗
We’re always busy with other tasks and PR reviews tend to be interruptions. Thus, it’s easier to spend too little time on a review rather than too much. If the purpose of the review is just to improve the current code, an in-depth review may seem less worthwhile. But keep in mind that code reviews improve the team too, thus the cost is easier to justify.
Thus you should probably spend more time than you are on your reviews.
The larger the skill gap between the author and the reviewer, the more time the reviewer should spend (thus helping to close the gap).
The amount of time spent during the reviews will also depend on the business risk and safety risk to patients. E.g., if you don’t have any users yet, the harm of a missed bug getting into production is low.
Review all the life-cycle stages 🔗
One of the key ideas is to review all of the life-cycle stages. In the next few sections we’ll examine how to review each stage.
How to review requirements 🔗
Reviewing requirements is difficult because, unlike the code, we often don’t have the full picture. Really, only the product owner has the full picture. Thus, usually the best we can do is check that all of the stated requirements (stored in the GitHub Issue or the Software Requirements Specification) have been met. We can also review that the requirements gathering was properly performed. Some engineers, especially more junior engineers, have a tendency to guess the requirements. You can try to determine whether this occurred, and make suggestions as appropriate.
We often have weekly sprint planning meetings. During the first half of these meetings we demo last week’s work. The main purpose of these demos is to have the product owner review our work and confirm that the requirements were met. These meetings provide a great opportunity to gleam information regarding the requirements, but we should remember to document this information so that it may be later referenced.
Occasionally you, as the reviewer, may remember some requirements detail that the author missed. This is most common if the project lead, who may have a deeper knowledge of the requirements, is the reviewer.
How to review requirements 🔗
Reviewing requirements is tough to do. Often, as software engineers, we can’t really asses if the stated requirements are valid. All we can do is make sure the stated requirements were met. You can also look out for gold plating, wherein someone implements more than was asked for.
Finally, you can check that the requirements were documented as appropriate.
How to review design 🔗
- Technique: (1) Read requirements (2) Sketch out how you would design a solution (3) Look at their approach (4) Ask questions to see why it is different
- Avoid being guided by the diff (often what is missing is just as important)
- Perform a risk analysis: how is the design most likely to fail?
- Check for appropriate documentation
How to review construction 🔗
- Is usually the best time to identify bike-shedding
- Generally, design is more important
- But don’t be afraid to be detail oriented
- Naming
- Pulling complex expressions into variables
- Whitespace (rely on linters as much as possible)
- Splitting a function
- If no skill-gap, selective in-depth discussions are good
- Be mindful of what’s missing (grepping can help with this)
How to review tests 🔗
- Risk driven
- Especially with junior engineers, encourage writing tests
- Code quality matters in tests
Use review checklists 🔗
Projects, especially regulated projects, probably have an explicit checklist in the development plan. We recommend using GitHub’s saved-reply feature to streamline using review checklists.
There are other things to check for here, including documentation.
When you’re feeling overwhelmed 🔗
Large pull-request reviews can be intimidating. Here are a few pointers
- Have the author walk you through the PR
- Use the “viewed” feature
- Try going commit-by-commit
How to give feedback 🔗
- Ask questions unless very confident
- Compliment good code
- If there’s a skill gap, give concrete suggestions
- If there’s a skill gap, suggest background reading (or 10x Lesson)
- Communicate “negative results”
- Don’t lower standards just because you don’t follow your own advice
Negative results: “I was worried about your approach to X, but I read this post online about it and it made me think your approach is appropriate. I also read through the other code in detail, but I didn’t find anything to comment on. It looks great!”
Don’t not give advice, which you know to be good, just because you don’t always do it. Engineering is about tradeoffs. Sometimes you need to write code that is lower quality in one way to save time. But there are other code quality things which are less about saving time in the short term. If you are suggesting something that you don’t always do yourself, ask yourself why you don’t always do it? Maybe there is some tradeoff guiding your decision? Or maybe you don’t have a justification, and this is just an opportunity to put pressure on yourself to do better. Also, your situation may not be the same as the author’s situation. Likewise, as the author, if someone suggests something they don’t always do and you don’t think it’s necessary, then have a discussion exploring why the reviewer doesn’t always do it.
Review status and final comment 🔗
Here are some best practices for your final review comment:
- Communicate when you’re done reviewing
- Communicate how long you spent or any shortcuts you took
- “Approved” means its ready to merge
- “Comment” means you’re going to continue reviewing more
- “Request Changes” means there is an important change to be made
- Sometimes its nice to pre-“Approve” to avoid round trip
Should I always leave comments? No. Sometimes there is nothing to say. It can be helpful to distinguish the case where you were too busy to do an in depth review from an in depth review that didn’t result in any comments. In the latter case, its helpful to comment on what you reviewed and any questions you asked yourself. For example, you could say:
If you just say “It looks great!” then the author may conclude you didn’t do a thorough review. As we discussed earlier, sometimes that’s okay, but you don’t want the person to think you were too busy or, worse, didn’t care, when in fact you left a careful review.
For example, if you recommends a technical book. Then, a few weeks later you get an email! “Thanks for the book recommendation; I read it and really enjoyed it!” You may wonder how carefully the person read it. If they read it carefully, wouldn’t they have said something more specific?
Don’t create issues where there isn’t anything to say. As you work with someone longer, the number of small comments will likely decrease. Eventually, most of the comments will be big-picture comments.
Responding to comments 🔗
Socrates: Thrasymachus, don’t be hard on us. If we are making any mistake in the consideration of the arguments know well that we’re making an unwilling mistake. If we were searching for gold we would never willingly make way for one another in the search and ruin our chances of finding it.
- If you disagree with a suggestion, make your case
- If you don’t think a suggestion is important enough to implement now, make your case
- IMPORTANT: Respond to everything
- A “thumbs up” is often sufficient if you agree
- Avoid justifying mistakes or poor decisions
- Request another review once you’re done responding
At Innolitics, we try to avoid absolute rules, but this is pretty close to being an absolute rule: respond to all PR comments.
Responding to comments. It is never acceptable to ignore comments entirely. This is disheartening to the reviewer. If you disagree with a suggestion, then make your case. If you think a refactor would make the code better, but isn’t worth the time investment, then make your case. Don’t ignore the comment. If there is enough time such that a review was performed, there is enough time to respond to the comments.
Fixing the issue vs responding: usually it is good to thumbs up, resolve, or write “fixed” in each PR comment. Why is just “fixing” the issue not sufficient? Sometimes there is a mismatch between what the reviewer thinks is being suggested, and what the author does. In these situations, one person may think they’ve addressed an issue while another person hasn’t.
Responding to comments 🔗
- Be gracious
- Once your questions are answered and you fully understand the situation, then make suggestions
- Make it clear when the PR should be merged using the GitHub review status
Merging your PR 🔗
- The author should handle the merge
- Avoid sneaking in new code
- If given pre-approval for a merge, be sure you’ve addressed everything!
- Delete the branch after the merge (GitHub can be configured to do this automatically)
Conclusion 🔗
I hope you can use the theory and practices in this article to improve your team. Here are the most important take-aways from the article:
- Good code reviews should balance improving the team with improving the code
- Review outputs of all life-cycle stages
- Ask lots of questions; avoid suggestions until you know
If you are building a medical device and would like help creating processes that comply with the FDA’s software guidance or IEC 62304 and also help you build quality software, we can help!
