over 3 years ago
The role of code review in the overall development process
- Leadership
Who should perform code reviews?
Anyone should perform code-reviews, not only senior developers or team/tech leaders. There are many benefits from letting teammates code-review each other regardless of their experience. To name a few:
- People learn best when they teach others.
- It eliminates bottlenecks on the senior developers. As code-reviews are split across teammates, senior developers can perform brief reviews before they approve the code changes.
- It improves communication between teammates.
- It increases the level of involvement of the team in the project.
- It increases the knowledge sharing in the team.
When is the right time to assign a code-reviewer to a task?
When developing features or change requests, we assign a code reviewer before the implementation starts. It is a win-win situation as the developers have someone to consult with about design & implementation, and the code-reviewers can provide feedback early in the process. When the code-review phase arrives, the code-reviewers are already familiar with the task and provide more accurate comments.
Phase 1 – approving the ticket
Before developers start to work on a task, they should first review the ticket and make sure it meets the standard we defined.
- Developers must write a well-defined ticket in a human-readable language. I remind the developers that any stakeholder who read it should understand the change, be it the QA, the team leader, or support.
- In the ticket, we group all the product requirements and UX blueprints, and we make sure they are written as simple instructions. To keep it simple and efficient, we defined a template for bugs and features.
- Everything should be clear from the ticket. The ticket is the single source of truth; developers shouldn't develop it if a requirement is not defined. If developers found something during the development, they must sync the requirements accordingly
- Although the product people create the ticket, the ownership moves to the developers the moment it was assigned to them so only they can update the behaviors.
Phase 2 – planning the design & architecture
There are many ways to handle the design phase. Some can decide to write HLD (high-level design) and TLD (technical level design), some can choose to conduct a meeting and discuss the design & architecture, and others can decide to jot a draft and start developing.
It depends on the task complexity and the developer's level of experience, and I let developers decide about it. Regardless they should keep the code reviewers in the loop and get conceptual approval from them. We do it early to avoid having design comments during the code review phase, which is way too late for those kinds of comments/rejections. It also reduces the developer's frustration as changing the design after implementation is usually painful, requires throwing code, and affects deadlines.
Phase 3 – writing code
During the implementation phase, developers should make sure they write based on the defined behaviors. Any code written should match a requirement; otherwise, it should not be written.
Any new requirement or change request discovered during this phase is subject to the developer's approval or rejection. They can decide to include them if it doesn't risk the deadline; if so, they should remember to update the ticket description to reflect the new behavior. If they decide that it will affect the timeline or disagree with the new requirement, they should escalate it to the team leader.
Tip - use unit-tests when writing your backend API
I cannot cover in a short, concise paragraph the motivation for having decent unit-test coverage. Still, I cannot overlook it as I honestly believe in it. So, I will mention the bottom line.
Although unit-tests are available in any framework, let it be in the frontend or the backend, I believe it is a must for backend API. When writing backend API, there are so many pitfalls to lookup for. As your code grows, any change you do in one place can affect your API's functionality in many unexpected ways, so, instead of relying on the code-reviewer, you should count on numerous unit-tests available at the touch of a button.
Code-reviewers must be able to run the unit-tests before they approve the code-review. They should also review the tests and make sure they address the relevant scenarios.
Phase 4 – conducting a code review
The ownership for approving the code is shared between the developers and the code reviewers. There is usually a misunderstanding about the boundaries of responsibilities, so that I will elaborate on it here.
The accountability for reaching the deadline with satisfactory implementation is on both of them. I used the word "satisfy" because we will never reach the level of perfection we wish for in the real world, and we need to settle on a code that meets the requirements, is overall well written, and will not likely introduce regressions bugs. We should opt to improve the codebase along the way, but we must remember the commitments we have for deliveries and deadlines.
TL;DR? What to expect from the code reviewer during the review process
- The code-reviewers' comments were productive, positive, and reasonable.
- The code-reviewers commented on implementation and business logic rather than code-preference or styling.
- The code-reviewers performed functional testing and verified that all the significant flows work.
- The code-reviewers confirmed each behavior in the ticket and resolved any discrepancy between the implementation and the requirements by either enriching the requirements or changing the implementation.
The developers who wrote the code depend on the code reviewers to approve their work, or otherwise, they can't submit the code. But as I said, merging the pull request is a shared cause. I am confident that my team members can balance perfecting the code and reaching product milestones. If any severe conflicts arise, they can both escalate to their neighborhood tech leader or manager to help find the right tradeoffs.
Checklist - things that should be in the code
The list below covers things that the code-reviewers should verify as part of the review process:
- Review each product behavior separately and make sure it is being handled as described in the ticket.
- Pay attention to edge-cases like errors during loading, invalid input from users.
- Consider real-time updates from other client software or users.
- Check for security concerns
- Ensure the changes are following the single responsibility principle
- Ensure calls between layers in the architecture are done in the right direction.
- Check for performance issues.
Checklist - things to shouldn't be in the code
This is a list of things that code-reviewers should comment about if found in the pull request:
- Code that was commented during the debugging and is not in use anymore and should be removed. The reason is that developers tend to comment outdated code instead of permanently deleting it until they are sure it is not relevant and it usually stays there.
- Code that doesn't contribute to the implementation and, if reverted/removed, will not affect the outcome.
- Changes that are not part of the ticket and were added to avoid creating new PR. If they are not part of the ticket requirement, they will not be tested by the QA, and they shouldn't be part of this PR.
- Changes that are not described in the ticket. You should either remove them or sync the requirements so the QA will test them.
Tip - perform functional testings as part of the code review for frontend applications
No matter how good and experienced the code-reviewers are, they are not machines, and by only reading the code, they will probably fail to find silly bugs.
The best tip I can provide is, you should have a way to run the application directly from the pull request immediately without the need to build it locally., a.k.a functional testing. For that, you must use ci/cd that integrates with your repository and deploy the application automatically for each pull request.
It is easier to say than done, but speaking from experience, once you do it the first time, replicating the solution to another project is super easy. As I already did it for four projects, I will share that solution in a dedicated post and add a link here once done.
How to tackle style and design comments
- Unless your team uses an established style guide, reviewers should focus on implementation and business logic rather than code-preference or styling. As we focus on implementation and avoid styling and personal writing preferences, the number of general comments is reduced, which improves the chances of a successful code review process.
- There are automatic tools that can be added to the process and enforce styling, and let's face it, a developer cannot get angry on a machine that dump 30 errors and 50 warning in the console when trying to push the changes.
- If there are style issues that are not covered by automatic tools, the code reviewers can raise awareness about it and escalate to the tech lead. The tech lead should either find time in a future sprint to extend the automatic tools and include those rules or gather them in a style guide document discussed and approved by all teammates.
- If there are design comments that we missed in the design phase, the code reviewer should start a discussion about it, but whether to fix it or not should consider the cost of rewriting and revising the design.
Thoughts about pair programming
I'm a believer in pair programming. Although it is not relevant to all developments, in some scenarios, it is highly beneficial. Having pair programming is an applicative replacement to code review. The only thing that the pair must do before merging is to review the changes together in the chosen code review service and follow the section "Checklist - things to shouldn't be in the code."
Thoughts about manners
The way you express yourself during code review has a significant impact on the developers' willingness level. Try to suggest changes instead of commanding about them, avoid criticizing how the developer wrote a code, try to be positive, and focus on the requested changes.
Suppose either the developers or the code-reviewers feel that the comments are too personal. In that case, they should immediately stop communicating over the code review and have a personal or virtual call to discuss it. Remember that chat messages miss the context and tone so often we can misinterpret the meaning and read it as sarcasm or criticism.
Consider working with a service named reviewable.io
There isn't any decent service out there for code review that I'm familiar with, excluding Reviewable.io. I talk with many developers about code-reviews and hear about many attempts to overcome this lack of services. But none of those workarounds works.
If you are using Github, you must give reviewable.io a try because:
- It is intuitive.
- It prevents you from merging until you resolve all the comments.
- It tracks revisions of files and lets you mark files that you already reviewed. Once you do that, you will only see changes on file level, which you didn't review.
- Comments of outdated code are not dismissed and are treated like any other comments.
- You can provide comments that are non-blocking comments and still contribute to the review.
Cover photo by John Schnobrich on Unsplash