Since github exists, They popularize the concept of pull request (Prs) when we ship code. In order to get the benefits of code review we use Prs as a way to review what we deliver.
In this process are involve, at least, 2 humans[1]:
A requester (Who ask for the CR), and a reviewer (Who do the code review).
But sometimes a reviewer can have a lot to review, a requester ask for somethign who requiere significant effort to understand, round trips between parts. So it ends with a lot of human time and effort.
This tips and hacks can help you to improve the speed of shipping code.
-
Get notified. Make sure you get notifications when someone call for your attention. This could be an email or web: https://github.com/settings/notifications or by our panda-bot: Pull Reminders for GitHub. If you don't like being notified you can check the menues on pulls, in particular review requested
-
Save some time in your weekly agenda in order to Review code. It is not mandatory do it all the time, but it is nice to practice it at least 2 times in a week.
-
Code review fundation is that with 2 or more devs, can increase the quality of the code. So, if you are pair-programming, you can merge your PR. [2]
-
Avoid large discussions/chatting as much as you can. Use code review to get or send feedback of what we are going to deliver. If the discussion start to grow, try to move in a different place, and left a link on the pr after. Face to face comunication is always the best strategy.
-
Make sure the pr has the rigth reviewers (or reviewers at all :P)
-
Avoid discussions about code style, on the same pr of a feature. Usually features are tied to a delivery date, we don't want to delay it. If there is a style you want to discuss, move the discussion to another pr, or at least different comunication channel. Involve your team, or engenering if is necesary.
-
Self code review. Check what are you shipping, if you want to see commit by commit use
git add -p, or open a PR, with awiptag. -
Clasyfy your PRs: Split your PRs in units of review, large code reviews are skiped very often, consider the ideas behind each PR, and how much it takes to the reviewer analyze the code. for instance:
Features - functional changes. Removing unused (dead) code. Structure refactoring - changes in classes, interfaces, moving methods between classes. Simple refactoring - may be done by your IDE, e.g. extracting variables/methods/constants, simplifying conditionals). Renaming and moving classes - reorganizing namespaces. Code style fixes - e.g. using autowiring, removing redundant doc-blocks. Formatting fixes. [3]
other examples colud be: - Translations - configurations of an a/b test. - autogenerated files.
-
If you have dependencyes between Prs, chain it. You can change the target merge branch, from master to your feature branch. so you can split in readable chunks. Once 1 is approve, can be continue with the next branch.
-
You have to push for a review, Prs are the oposite of whisky. Don't get better when aged.
-
Stick your comments in to the code, and see if you see potential bugs, or functional gaps.
-
If you don't see nothing wrong, or bad. Don't heassitate to approve. If you want to left a feedback describe what you understand of the code. If you not are sure call for super code review (more eyes)
-
Try to avoid procastination or "leave it for later". If you are not able to compete the review (because reasons), at least write some notes. If it is not for the requester, them would be useful for the next reviewer. Remember to point to another reviewer before jumping off.
-
Start from the tests. This help you to know the domain and the problem we try to solve.
-
Use your checks on the right of each file: this helps you in 3 things [4]
- remove from your view, chunks of code you alredy review
- remove from your view autogenerate files
- it will appear unchecked if the requester send you new changes.
-
Use code suggestions, there are very usefull. And speed up the requester changes. (proceede with caution this could conflict on code styles or linters)
-
Avoid linter comments, we have linters for that. If you want to add a new rule , try to discuss on #engenering or #webpeople
-
Get involve in the resolution of your suggestions, push for that.
-
Indentify and clasify your feedback:
- If you have a deal breaker, use "request changes".
- If you only have feedback to leave, nits, or just soft rules. Use "comment".
Some times, the request change is not dismissable by the requester (permisions) and can't be dissmis until the reviewver approves.
[1] Applies to replicant. if you are not sure, take the test: https://www.bfi.org.uk/are-you-a-replicant/ if you don't what is a replicant watch: https://www.imdb.com/title/tt0083658/
[2] https://blog.codinghorror.com/pair-programming-vs-code-reviews/
[3] https://sergeyzhuk.me/2018/12/29/code_review/
https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/