Code Review at Frontline
Nowadays, code review is standard procedure of development process in many companies as well as it is in my company and more specifically in my team. We have been using Bitbucket Server as source control versioning platform and it already supports code review process in place as a standard feature; however many other platforms, one way or another, support the same feature in place. Anyway, you don’t even need any special tool or whatever. A tool may just makes it easier and friendlier in everyday life.
In this blog post, I will try to visit the purpose and potential benefits of code review activity and share some quick tips. All these statements are subjective to my experiences and I would love to hear more about yours. Please feel free to post below :)
A Code Review activity requires two parties as The Code Committer and The Reviewer. Both parties involve one or more people but it should never be the same person on two sides of table!! For sure, many people would agree with this last statement but sometimes pair programmers tend to approve (not same thing as the review) the commit which they have actually committed together. You may think that, a pair’s commitment would be good enough in many cases; however, as many engineers would say, it depends!! If you don’t see code review as an optional activity, a safe option would be to always ask an independent party to review the code.
Every developer commits some piece of code; but a common question might be whom to review it. For my team, answer is quite simple: any other two other developer in the team, one should be preferably comfortable in domain. Everyone’s commits are reviewed by some other developers and it is no matter what title (we don’t have titles in Trendyol, everyone is just software engineer) nor experience the committer has. I believe this is an important criteria, because it makes all that process fair and sincere for everyone. I specifically ask for review from everyone and try to get benefit of it then and in the future. It is also a way of learning for me.
So, we bother each other with code reviews and spend our valuable time on it. For sure, it must serve to a common purpose in the end. For me, a short list of benefits can be:
- Potential opportunity to learn from others. I have seen developers voluntarily looks for code changes and reviews just to learn more about.
- Achieving standardization in code and way of working
- More eyes lead to less errors. A (more) stable code at first place should be natural outcome in this case.
- Contribution to shared code ownership
- An independent review of the solution (not just the code)
Here are some tips, which I find useful:
As a committer,
Don’t take things personally. It is your code reviewed, not you. Honestly, there were times I felt offended by some comments to my code. Just take it professionally. Don’t forget that reviewers are also humans and sometimes a comment may not be right or fair. In the end, you don’t have to agree with all comments, but always remember the fact that, code belongs to team, not just you. So it should be embraced by everyone.
Keep commits as small as possible. Reviewing a very big change can be tedious and exhausting. Frequently commit your code and ask for a review. Reviewing an incomplete commit may not be a good option (can also be valuable and early feedback) in some cases; however, try to find the right balance for reviewable code size. Don’t underestimate the power of an explanatory commit message and description. Sometimes it means a lot.
Don’t distract the reviewer with optional and context irrelevant changes. We write code and have a mission in a specific context. Make your context irrelevant changes (like a small refactorings in visited code piece) in separate commit so reviewer can focus each mission separately. A specific example of this statement can be, removing unused import statement in a C# code. If a committer removes all unused import statement in a codebase along with her changes, reviewer would find a long list of files in the Pull Request (this is how we integrate changes in our way of working). Another example would be to auto formatting a large document. Just leave it in a separate commit if you would like to do that.
As a reviewer,
Take your time.
Be gentle to express your statements in your comments. Be specific with your arguments and concerns when you request a change. A change is costly, so it should be worthwhile to spend.
Don’t skip unit tests. Make sure tests serve to validation of changes. It is also a very handy way of understanding what the change is about.
Don’t seek for the perfect. Perfect is enemy of good. Initially, focus on working code. We can all move on to life with an extra whitespace but a review should validate solution in first place. Then we can discuss how code can be more prettier, readable, extensible or maintainable. For sure, these are all important measures of quality.
Be mind opening rather than directive.
Use static code analysis tools in first place. All of our time is valuable and many common code review concerns can be automated in build pipeline. Use a static code analyser and integrate it into your build pipeline. Configure your static code analyser properly and make there are no major issues left before starting to review it. So, we don’t have to worry about a misspelled variable name or etcetera.
