How I typically do code reviews
I don't know about you, but I enjoy code reviews. There's something about digging into a premise someone has worked up in their minds, agreeing or disagreeing with the premise, and then working myself down through the details, trying not to die on any of the pet-peeved hills I don't care too much for. I also like submitting code up for review, lately it's allowed me to see many different reviewing cultures from different developers around the world.
Here's a snapshot of the thought processes that I think about actively and try to apply during code reviews, both from the reviewer's point-of-view but also from the submitter's perspective.
As a reviewer
I will initiate a review in some of the following ways:
If the request-for-review is on a general channel on Slack
I may post my intent to read it through.
I may assign myself to the PR.
I may attach an 👀 emoji to the original call-for-review post.
If I get asked directly for a review via a DM I'll try to respond with an expectation on when the review will start.
While I'm doing the review:
I try to read every line of code.
If I don't understand something, I'll post a question about it. I'll stash the comments into the review "buffer" until I've done the whole review and flush the buffer only at the end, sometimes with some kind of general comment to the merge request about the overall premise.
I try to write my questions/comments/suggestions in such a way that they can be independently resolved. Made-up example: "Should this field be nillable? This comment can be resolved with an explanation or a fix."
- In theory, this allows for independent code review resolution. In practise, you usually need to clarify some of the wording you are using or some point you are trying to make. But I still try do it for blocking questions anyway :)
I use blocking and non-blocking review comments.
A blocking comment should be addressed to proceed with the review. A resolution may sometimes be an answer to a question, but it can also be a fix, or it could be a quick investigation about the underlying assumptions. As mentioned, I try to think of a way of how the submitter can independently resolve the review comment to allow for a single-pass code review.
A non-blocking question will contain a string such as
[nit],[aside], or[minor]. These don't have to get addressed to merge the PR. They are things that you want to say out loud in order to spread knowledge or to discuss, say, about a semi-related topic that isn't a problem now but could be in the future, or to propose a minor code change that the reviewer feels could be important from their perspective. It can be[praise]too :)Nowadays I try to avoid adding non-blocking comments because of how GitLab works by hindering the merge process when there are any unresolved comments. GitHub however is more lenient in allowing the review process to continue, keeping unaddressed comments as unclosed.
If there are multiple blocking issues with the code, I've typically ended up doing a synchronous discussion about the design in general.
Avoid drive-by commenting, that is, adding review comments where I don't see the review through with approval/rejection.
Nowadays, I think a lot about the hill that I want to die on. It's okay not to have an opinion on everything.
After I've finished the review:
I'll mention about it in the original venue the review was asked.
If I rejected the PR, then the review issues can be independently approved by resolving all of the blocking comments*. However, I'll still need to approve the addressed pull request.
Once I've approved the PR, I'll typically exit the process. The submitter will merge the pull request when it's suitable for them. But also, the review may still be in progress (by other reviewers).
As a submitter
Before I submit code up for review:
I prepare my diffset (code, clear-cut commits with constant use of
git add -p, documentation etc.)I try to prepare my diffset in such a way that it doesn't contain radically different topics. Pragmatically speaking, adding Makefile improvements, pipeline fixes, or quality-of-life improvements are good in my book, also sometimes it's hard to split things up.
I may try to see that my diffset is small enough to be understood and thus reviewed. Sometimes I may split a PR in pieces either ahead-of-time or right at the end.
Wide auto-formatting together with functional changes is a big no for me. Pragmatically, I've accepted that the immediate or near-to-immediate surroundings of the code I touch may get formatted (eg. function-level scoping).
At the end, I re-read my code (every line of it) and fix anything I spot.
Asking for a review:
This greatly depends on the social agreements of the team, but I generally go to the team dev channel and post the link to my merge request. Sometimes it contains a short description of what it is, sometimes I use a clickbait title. No hard and fast rule to it.
- While this works I've noticed that I have a bit of a difficulty getting my code reviewed in a short amount of time. I'd really like to see how this could be improved, be it developer process or a general change in the way we communicate with people daily etc.
Addressing the review:
I will address all blocking comments.
Most likely I'll address all non-blocking comments too (golfing suggestions aside).
I'm fine with either synchronous or asynchronous handling of the review if it gets the job done, though I prefer explicit commenting.
That's it! That's how I try to do my code reviews. It doesn't always go exactly according to what I described here. But if you stop looking at this as a strict playbook and more of as something that I think about a lot during code reviews, then maybe it doesn't feel so checklist-y and rigid.
Ultimately, the only thing that matters is that everyone involved understand what is going to be merged in and that you, as a reviewer, commit to see the review through together with the submitter.
* All of my advice in this blog post applies to an experienced team. Care should always be applied whenever the submitter is touching code parts that may have an irreversible effect on the production environment.