Review Code Like A Human
Reviewing code is challenging and time-consuming.
The fact that everyone has their own style and the lack of standards, add multiple levels of complexity that lead to more time waste and sometime, even frustration.
This blog post will present my research findings on the things you should look for while reviewing code and the things you should not. Hopefully, these tips will help you become a better reviewer and make the process a bit less oppressive.
First of all, we should recognize what is the goal of a code review. It happens that developers often assume that the goal of a review is to reject the code and state all the reasons why it can't go into production. I want to deny this claim, our goal as reviewers is to make sure the code is good enough to go into our production code. When we review, we should try and understand the flows and logic in order to help the submitter make the code more readable and understandable. After all, these two factors will impact your team's code growth and maintainability the most (assuming the code also works).
Things you *should not* focus on while reviewing code.
Humans should not focus on anything that can be automated.
- formatting.
Most IDEs have built-in tools to format your code. IntelliJ for example, you can set code style schemes to match your team code style. If your team does not have a code style file, using google's default styling is a good start. (google's Java IntelliJ style guide ) - Naming.
This is another matter that can be automated. For example on IntelliJ you can set naming checks and conventions, to match your team code style.
I have seen that most developers don't use these features so if you would like to enable them, go to Preferences | Editor | Inspections | Naming conventions
Avoid these anti-patterns while reviewing.
- Nitpicking.
Our goal is not to reject the code, not to show how clever we are, and not to force our code style on others. So nitpicking is out of place while reviewing code, it will do more harm than good to your team. Instead of nitpicking white spaces/tabs, formats and naming, spend some time formulating standards for your team. Translate these standards into style guides and let this process be automated. - Inconsistent feedback.
I had this one a lot in the past, either inconsistency between different reviewers or even inconsistency from the same reviewer (when you submit code and, a reviewer requests changes, you make them and the reviewer comes back with other changes.)
It can get frustrating pretty quickly and in general, it is a waste of time. If you submitted changes as a reviewer and find yourself writing more comments after the submitter has done the changes you requested, there is some problem in your reviewing process. - Ping-Pong review.
For me, this one is the most frustrating.
If you see that a change request requires too many back-and-forth interactions you should schedule a short talk with the PR owner, talked about your concerns and solve the issues together. - No response after a review request.
This one happened to all of us, We were in a middle of a task of our own and could not afford to context switch. But, you should try to reflect back your status to the requester and present a time frame you'll be able to look at the code. - Request a design change too late.
Most of us will meet our peer's code when it is in its final form. That means your teammate has already put in the time and effort to create this PR, he possibly talked over the general design with someone else and you weren't in the loop. With that said, it is too late to request a major design change. If you find yourself requesting that, some changes need to be made to your team workflow structure. Generally, the design process should be separate from the implementation process.
Things you should focus on while reviewing code.
- Check the code is understandable.
This one is the most important in my opinion. Understandable code will be easier to maintain, scale and debug. - Make sure the code meets standards.
Most teams have their own definitions and standards. For example, your code is using Java streams and not for loops, as a reviewer you should spot these kinds of things. If you don't have these criteria in your team you should sit together and assemble them. - Share your previous knowledge.
More than once I encountered one of my peers facing a problem I already tackled and implemented a solution for. This is a very good place to suggest your findings and improvements. - Find Bugs.
Humans are not so good at finding bugs. Automated tests should take care of it. But not all teams have wider enough test coverage. In that case, you should try and understand what the code is trying to do and point out potential bugs.
Reviewer tips.
- Try to review your peer's code as soon as possible. Each minute a code is waiting for review and not being used is considered a waste since it is already implemented and your clients cant enjoy it.
- When approaching a review you should have a checklist of things you want to walk over. This will also help you achieve higher consistency in your reviews.
For me its -
Check the code is understandable.
Make sure the code meets standards.
Share your previous knowledge.
Find Bugs. - Be nice! You and your teammates are on the same team. Point out good things on the change request as well. Criticism can make people uneasy, but pointing out well-done objectives can help them accept the bad comments better.
Submitter tips.
- Pull Requests should be small.
10 files are considered to be the limit. Studies show that the longer the pull request the fewer comments you get on it since your reviewers get overwhelmed by the amount of code. assuming this is not your goal ;) you should keep them as small as possible. - Add comments at key points of your code. [to the PR not in the code!]
for example, I changedsave
on line 62 tosaveEntity
, this change can be confusing to a reviewer who is not in all the details so I added a comment to explain the change.
Use this kind of comments if you are refactoring names or making style changes, it will save your peers a lot of time.
- Add a review flow for your reviewer to follow.
With this list, the reviewer can follow the submitter's train of thought and the PR suddenly makes much more sense without the need to do the reasoning yourself.
Conclusion
Code review is our best tool to ensure proper code makes its way to production. Although this process requires time, soft skills, and experience,
we can make our reviews more efficient and consistant.
Some of the tips you read here are easier said than done, but applying them can take your team and your code review experience to the next level.