Best Practice of Code review — Review and be Reviewed
To Review or to be Reviewed,
That is not the Question,
That’s Our Daily Job
For a newly programmer, Code review seems like a scary process.
Being corrected and checked for all the things that you push.
Who knows what critics might come in the form of request changes comment ?
Some might think that this is only a formality process.
Some might think that this is just a standard procedure.
But is that all ?
But the code review process is not that simple,
it is quite a beneficial yet could build the culture of the team ,one pull request at a time whether for the reviewer or the reviewed
1. Why we have to ?
- Standardized the code quality, consistency and minimize human error
Why are you using the recursive for fetching the data ?
Why not move the process in the background ?
Just use my extraction function in the utils package, dude.
Whether it is for the first or old timer, any review process will help in maintaining the quality code. every services must have their own standard and guideline development and sometimes, even those who create the standard will makes an error in his pull request.
That’s the job of the reviewer to make sure that every changes is a part of maintaining the consistency of the code.
- Sharing knowledge in a form of code
When doing the review, we could see much more about our peer.
What task he/she have ?
What problem he/she might encounter ?
What solution does he/she comes up ?
Any changes is always a part of discussion,
We can discuss about the solution they have with the solution we have,
either way, it is a correct place to start an argument that which lead to both sides having decision and knowledge.
- Motivating commiter
Rather than giving auto-approve for every pull request,
Checking the code throughly and finding the other’s mistakes might be better things to do.
That means, we are really care for what our peer did and allocate a times to notice a things that he/she might learn or correct.
This will strengthen the bond between team member where each of them got each other back.
2. What to Review ?
It’s harder to read code rather than write it — Joel Spolsky
These might be the list of the most common things to check in every code review
- Does this code achieve the committer’s purpose ?
- Any potential abstractions or is there any duplication ?
- Is this already follow the services standard pattern and guidelines ?
- Is there any other more efficient way to solve the committer’s problem ?
- Is it easy to read and easy to understand ?
- Is it still have any TODOs ?
- Does the committer already complete the test files correctly ?
- Does the committer already updates any external documentation or readme files?
3. When to Review ?
Ask a programmer to review 10 lines of code, He will find 10 issues.
Ask a programmer to review 500 lines of code, He will say it looks good.
The best and most optimal time to review a code :
- 400 LOC per session
- Under one hour per session
- If it is more than that, Do split the session
Since the more code you read, the more you are prone to ignoring the mistakes.
4. Reviewer Tips
- Equality Review. Review the code ,not the coder
Whether it is an intern, engineer, senior engineer, lead, manager, vice or even the CTO himself/herself.
Every code is an equal code and must be checked throughly and equally. - Give explanative feedback/comment or question
Make sure that the committer knows what you want to say - Learn what the author’s doing and how the author’s think thorough his / her code
This will get the big picture of the whole pull request and helps us understand better the intention of the committer - Foster a positive code review culture
Don’t say any harsh word or blaming the committer, this might set the process of code review as a bad experience for a newly member. instead, use the process as a part of creating a constructive culture between engineer. - Be responsible when you are giving a comment / request changes !
I had a case where my pull request is given a request changes and the reviewer suddenly took a leave for a week.
Reviewer still have the responsibility for checking again after the request changes has been solved, so he/she won’t become the blocker for the committer.
5. Committer Tips
- Do try to compile and execute all the test
- Do self-review first
- Give the most explanative but short yet concise commit message
- Choose your reviewer
- Submit the code that only review ready and don’t change the code while review in progress !
- Open minded but don’t be a “Yes Man”.
In Summary
As simple as the code review process is, it defines the culture and the healthiness of the team. Whether is it a constructive and positive interactions,
or is it a tense nerve-breaking process that slowly decreasing the productivity.
It is a job for every team member to make sure that this code review process become a positive culture that someone can always learn a thing or two while still maintaining the quality of code.
As always, we have an opening at Tokopedia.
We are an Indonesian technology company with a mission to democratize commerce through technology and help everyone achieve more.
Find your Dream Job with us in Tokopedia!
https://www.tokopedia.com/careers/
