At Applidium, trying to increase our code quality, we decided to establish code review in our projects. We are definitely convinced it helps producing better code and gives developers an opportunity to learn more from each others, both by getting reviewed and reviewing.
Why use code review?
When developing a software, bugs are unavoidable. That may be during development, Q/A tests, or worst, after release, yielding a bad user experience.
The SmartBear study about code review shows that even a quick code review is efficient enough to find most bugs during development. This reduces the amount going through to Q/A and customer, which costs way more money.
Briefly, code review helps to:
- Improve code quality
- Reduce defects in code
- Improve communication about code content
- Educate junior programmers
This indirectly means:
- Shortened development/test cycles
- Reduced impact on technical support
- Increased customer satisfaction
- Creates more maintainable code
- Increases reviewer knowledge about code quality as he or she needs to formalize quality criteria
How do we code review ?
- -2 This shall not be merged
- -1 I would prefer this is not merged as is
- 0 No score
- 1 Looks good to me, but someone else must approve
- 2 Looks good to me, approved
There are some points a reviewer should cover to handle review properly:
The first criterion is “does the commit respect the company coding style?”. Indeed, the coding style is a set of rules derived from industry standards made to increase understandability. Invalidating a change because of syntaxical details may sounds strict, but having everybody respecting the same syntax insures a good readability and maintainability. This step can be automatised by continuous integration. At Applidium, we use SwiftLint for iOS Swift and Lint for Android running on Sonar to automatize this verification.
Is the code clean and safe?
This is general observation about code quality. Is the code SOLID? The reviewer should also check for code duplication, missing error handling, etc.
You should consider the safety of API changes: for example, a new method may be used properly within the reviewed change, but, at the same time, it may also sneakily give someone else an opportunity to misuse it later.
Any change is also an opportunity to clean old dirty stuff, to leave the code in a better state. Every increment matters, even the smallest.
Is the code functional?
Looking at the syntax, and general code quality is not enough, the reviewer should also check functionalities, look for potential bugs or threats. Looking at its smartness is also a good idea, checking for efficiency is a good way to increase code quality as well as instruct programmers.
Is the code understandable?
The reviewer should be able to understand changes (and more generally, the code) on their own; if he/she needs some explanation from the author, it means other developers won’t be able to understand it later.
Good method/variable naming, class/method length or method argument count are some code understandability criteria.
The first reviewer is the change owner, before submitting a change to others he should himself proofread it (TODOs, comments, debug variable, and all the previous criteria). To ease reviewer work, this is important to keep the commit flow logical.
Play with the feature
In case of reviewing a feature spanning over several commits, it is important to consider each commit separately, and then the whole feature. It is good habit to checkout the completed work, look over the code, and follow its execution, both mentally and with the debugger. Try to imagine how you would have implemented the feature too. Comparing your architecture with the actual one may help you to understand the implementation and its efficiency.
Take your time
Do not treat code review as a mechanical obligation, this is an important part of development. There is no interest in rushing to get it done. If you’re not ready to spend enough time at it, it’s better to postpone.
Should you read all modifications?
In some cases, changes may look obvious and redundant (for example renaming a project-wide variable, updating a resource, etc.). It’s up to you to decide whether all those files are worth reviewing. To make up your decision, it is important to remember that obvious changes may contain careless mistakes, and the previous point: if you are not focused on each file, then this is a waste of time.
Keep the exercise friendly
Keep an open mind, when reviewing, and when receiving a review.
Comments should always be explained: why it’s not acceptable, and how to improve it.
People do not generally like being told what to do. Explaining your comments and asking questions yield better chances to get a coworker interested in being reviewed and accept your comments. On the other hand, do not take it personally, this is not you being judged, just your code getting reviewed. When receiving a comment stay open minded, this is how constructive discussion happen.
Finally, of course, it is totally possible to give positive comments!
Do not drive the reviewer crazy
As the reviewer will probably read your stack of change as a story, he or she won’t appreciate if you change your mind (and the implementation of your feature) three changes after it has been written. It is important to keep a clean and consistent history through the feature implementation so that the reviewer is able to read and understand it properly. Don’t hesitate to fixup, squash, and rewrite your history before submitting it for review!
Reduce the “grey zone” discussion
Grey zones are those subjective topics on which two co-workers may have different opinions, but both could be right. Arguing about such topics often lead to unconstructive conversations during which no one wants to change his mind. This is why it is important to explain comments, it often helps to find legitimity. In case you think a change is not properly done but you cannot clearly explain why, you should consider it as acceptable.
Once your team agreed on a new grey zone rule, it is important to update your rules and notify the whole team about the change, and its reasons.
Your are allowed to be unconfident
If you are not sure about a change, it is still possible to give it a +1 grade (Looks good to me, but someone else must approve). This is better than validating a change that you feel like you don’t fully understand, or whose implications are not totally clear to you.
This is a team effort
Reviewing and handling commits and branches become insanely complicated as the number of developers working at the same time increases. This is why it is important that everyone stays concerned with reviewing others and taking care of their owns changes.
Code review may look time-consuming, useless, or even boring. In fact it is, unless you do it in order to learn new techniques, care about details, be positive and motivated about it. It is necessary to dive into the change, consider whatever it implies, communicate about problems or good coding habits the reviewer/reviewed couple may encounter. This creates a good way of sharing knowledge. At Applidium we consider code review as a great opportunity to increase code quality and developer skills, and therefore, as a key step of development.
Let’s continue this discussion !Contact us !