The code review is the act of systematically and consciously convening with teammates to check each other’s code. It helps to detect problems at an early stage. Generally, the code reviews apply for pull requests.
Cross-review is one of the best practices. Cross-review is a code review when the PR’s author and reviewer are different people. The reasons why cross-review is really effective are the fresh look and different targets for the author and reviewer.
The PR’s author is always sure that his code is perfect, but the reviewer is sure that the code still has improvement points. Later in the article, we will analyze these two roles separately.
The Author Role
I want to start with the author role because many guides skip it, but code reviews are communication between two or more people. Actually, you can change your role from PR to PR, and it’s typically practiced for IT companies.
Don’t waste your teammates’ time
When you’ve prepared PR, don’t forget the next steps:
- Repeat all needed use cases for your code.
- Try to break your code
- Try to find edge cases
- Check your coding standards
- Run tests, code quality tools, and building tools.
The main development goal is resolving the business problems. Therefore, the perfect code that doesn’t work is garbage.
Criticize your code
- Have your changes fixed the problem?
- Have you anything extra in the code?
- Is your code simple enough?
- Can you simplify the code?
- Have all coding standards been met?
You can add extra questions to the list looking at your special skills.
Check the PR description
Remember, reviewers often don’t have the same background and deep-diving into your issue, so you should read the issue and PR description and be sure that it’s enough to catch on which problem and resolves your code.
- Do you have any specific information for sharing?
- Have you added test cases?
- Have all CI/CD processes been passed?
How to handle reviewer comments
The essential point doesn’t take it personally. The reviewer’s goal is to help you to make your code better. If some comments sound not joyful enough for you, that’s okay because learning is a complicated and painful process. Don’t forget that the reviewer’s goal is to help you.
Try to fix all suggestions. If you think that the reviewer suggested the wrong solution, try to convince him and add more detail with a small research and compare your and his solutions.
When you are sure that all reviewer’s comments were implemented, then double-check the reviewer’s comments again and be sure of your code’s working capacity.
Remember that fixes after the review is a piece of brilliant advice from your teammates, and attentive work on them is your point for growth.
The Reviewer Role Principles
A really responsible role that needs a lot of your attention to detail, soft and hard skills. Let’s start with general information that is really important to understand.
Code should be better, not perfect
Hopefully, you understand that coding is a daily routine and code becomes better with your skills growth. From this, we can conclude the code that was written a few months ago cab be improved today. Technical debt is becoming the technical mortgage really quickly. And your team should quickly run to become a bit better.
A lot of engineers have a perfectionism syndrome that often isn’t needed here. And when the author and reviewer have the tendencies towards perfectionism, it increases project delivery times and often disrupts deadlines. So to avoid, remember one simple rule:
Done is better than perfect.
Someone wise
The main target of your code is to deliver business features as soon as possible. Most managers understand that quality code needs time for documentation, tests, coding standards, etc., and it’s okay. But complicated architecture for marketing features with unknown future it’s overengineering and sometimes better to prepare the quick solution and refactoring it in the future.
Coding standards
The coding standard is the most important component of teamwork. You can read more detail about coding standards. I really like the phrase from the Google Code Review documentation:
Coding standard is the absolute authority.
part from the Google Code Review documentation
Personally, I share this position, but it’s important to bring it up gently and unobtrusively.
Non-critical enhancements
If something isn’t critical but nice to have you should mention it. Google’s developers suggest using the nit prefix.
nit: short for “nitpick”; refers to a trivial suggestion such as style issues
Chromium Glossary
If PR has only non-critical conversations, don’t waste time and send it far to your project pipeline.
Technical facts and data overruled opinions and personal preferences.
I believe that everything is clear here, even without comments. Try to offer technical facts to show that it’s not only your desire.
Mentoring
Code review is one of the biggest touchpoints with your team. When you help somebody to grow, you help firstly for yourself – the win-win strategy. Don’t forget, when more your expertise than more you can share your knowledge. And it’s okay when people are on different levels of expertise, and you should help them grow.
Secondly, don’t forget when you criticize the code, the author can take it to heart. So, be more polite. You have time to rephrase feedback.
The next point is to attach more details. If you know, something doesn’t guarantee that the author is on the same wave and exactly understands your request. Imagine that you try to explain something to the child. Detailed explanation with technical data gives the author care and positive emotion after review.
Last but not least, don’t share the code that resolves this problem because it’s a disservice. Better to share some examples concerning how you prefer to see the result. I know that isn’t very easy, but it’s also a skill that you can improve every day.
What Should The Reviewer Check During Review?
The Design Solution
In general, is the direction of the solution correct? Is the best way to resolve this problem? Does the solution have a good architecture? Is the code in the needed place? Is the solution has enough abstraction to be scalable in the future?
Functionality
What will it give the customers? Is it comfortable enough and simple to use? Is it working as expected? Try to find the edge cases and break the code like a customer.
Security
Is the code free from SQL Injection, XSS, CSRF, Race Conditionals, etc.?
Complexity
Is the code easy and quickly to read? Do you feel the overengineering smell?
Tests
Hopefully, you know why automated testing is an important process.
Are tests written for the current PR? Are the tests running successfully? Do the tests conflict with tests that were written earlier?
Naming
Do variables, methods, classes, functions, constants, etc. have a good and understandable naming?
Comments
Note, comments are not documentation. About documentation next.
Is this a necessary comment? Is there literate and good English?
The good comments should describe “Why” they exist, overwise the bad comments – what does the code do? The developers know and can read what this code does without any comments.
Documentation
Classes, methods, functions, etc., have documentation blocks. Each documentation block has a good description, correct tag (@since, @param, etc.), and valid input/output data types. If you are using some external documentation, also check it.
Issue & PR description
Issue and PR description are really important because it’s a short description of a completed problem. If someone wants to check it next year it should be easy to understand in detail.
Double check every line
Read PR’s every line and try to find something to improve. Ask more questions that were described earlier.
Summarize
The code review is really important for the great development process, and you should be attentive to it. Hopefully, during the reading of this article, you’ve found something interesting for you. I feel that the code review process is still or maybe more important than the development process. Have great code reviews and interesting projects.
