Since GitHub became popular, so did the concept of pull request (PR) for shipping code. In order to get the benefits of code review we use PRs as a way to review what we deliver.
In this process at least 2 humans are involved[1]:
A requester (who asks for the code review), and a reviewer (who does the code review).
But sometimes a reviewer can have a lot to review, or a requester can ask for something that requieres significant effort to understand and round trips between the two parts. So it ends up costing a lot of human time and effort.
This tips and hacks can help you improve the speed of shipping code.
-
Get notified. Make sure you get notifications when someone calls 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 menus 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. [Demian note: creo que lo mejor es recomendar priorizar los code reviews antes de la tarea propia, ya que haciendo code reviews siempre destrabás a otra persona :)]
-
The core idea of code reviews is that having more than one pait of eyes looking at a code change, the quality of the code can be increased. 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 starts to grow, try to move it to a different place, and leave a link on the PR afterwards. Face-to-face communication can be swifter on these discussion-heavy code reviews.
-
Make sure the PR has the right reviewers (or some reviewers at all :P)
-
Avoid discussions about code style on the PR for a feature. Usually features are tied to a delivery date, and we don't want to delay them. If there is a style you want to discuss, move the discussion to another PR, or at least different communication channel. Involve your team, or engineering, if it is necessary.
-
Self review first. Check what are you asking to be reviewed before bothering others [Demian note: no sé si éste era el espíritu de este punto, pero el resto sobre
git add -pla verdad que no lo entendí :P] -
Classify your PRs: Split your PRs in units to review. Large code reviews are skipped often. Consider the ideas behind each PR, and how much it takes the reviewer to 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 could be: - Translations - configurations of an a/b test. - auto-generated files.
-
If you have dependencies between PRs, chain them. You can change the target merge branch, from master to your feature branch.
-
You have to push for a review, PRs are the oposite of whisky: the don't get better with age.
-
Stick your comments into the code, and see if you see potential bugs, or functional gaps. [Demian note: este punto no entiendo qué es lo que quiere decir :S]
-
If you don't see anything wrong, don't hesitate to approve. If you want to left feedback, describe what you understand of the code. If you are not are sure, call for super code review (more eyes) [Demian note: los "super code reviews" son una cosa en tu laburo? Si no, no se entiende qué es eso me parece. Y, por otro lado, el wording de cómo empieza este punto me parece un poco "al choque". Onda, si no veo nada malo en el código, pero tampoco veo el motivo por qué se quiere agregar, no voy a aprobarlo, el código siempre tiene un costo]
-
Try to avoid procrastination or to "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, they would be useful for the next reviewer. Remember to point to another reviewer before jumping off. [Demian note: muy buen punto! :D]
-
Start from the tests. This can help you to know the domain and the problem we are trying to solve.
-
Use your checks on the right of each file: this helps you in 3 things [4]
- hide chunks of code you have already reviewed
- hide noisy auto-generated files
- it will appear unchecked if the requester sends you new changes.
-
Use code suggestions. They are very useful. And they speed up the requester changes. (Procede with caution, as these could conflict on code styles or linters)
-
Avoid linting comments; we have linters for that. If you want to add a new rule, try to discuss on #engineering or #webpeople
-
Get involved in the resolution of your suggestions, push for that.
-
Identify and classify 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".
Sometimes, the requested change is not dismissible by the requester (permissions) and can't be dismissed until the reviewer 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 know what a replicant is, 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/