about 1 year ago
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:
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.
Before developers start to work on a task, they should first review the ticket and make sure it meets the standard we defined.
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.
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.
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.
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.
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.
The list below covers things that the code-reviewers should verify as part of the review process:
This is a list of things that code-reviewers should comment about if found in the pull request:
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.
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."
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.
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: