This article discusses how to code review within a pull request in order to improve your code quality. Let's start with pull requests first.
What are Pull Requests?
For those unfamiliar, pull requests are used to get peer approval before changes are merged within a version control system, like GitHub, Bitbucket or GitLab. You can find more documentation about it on GitHub, Bitbucket and GitLab.
This approval typically comes after the code is reviewed and no further changes are requested. This process is often referred to as code review.
Bear in mind adding code reviews to your development process is quite time-consuming, meaning management has to be okay with your weekly hour expenditure on code reviewing.
Today you can add automated code review tools to your workflow so you can get even faster code reviews. Go ahead and type automated code review tools on Google and select your favorite one.
Integrating code review into your development workflow
Having code reviews in your development workflow certainly constitutes a process and many dislike it.
Among the reasons I’ve seen listed by people who dislike the process: 1) they consider Pull Requests a step to validate the technical details they’re sure are correct 2) acquaints other with the code they wrote 3) hurdle getting in the way of what they love doing: coding.
I’m going to argue otherwise.
Quoting GitHub documentation: “Reviews allow for discussion of proposed changes and help ensure that the changes meet the repository’s contributing guidelines and other quality standards.“. Thus Pull Requests are a medium for discussion. The discussion implies questioning, debate, iteration back and forth for the sake of a better codebase.
The point is to both improve the code and improve all of the contributors’ understanding of it. You’re trading iterations for more polished code and increased overall knowledge.
Of course, this way of thinking also annoys any programmers who dislike communicating and/or discussing code. And though every human being differs in the topics they enjoy talking about, or the amount of interaction they are comfortable with, in the realities I came across as a professional developer, programming has always been a team sport.
Comments
There are two things to keep in mind when commenting:
There are places where the code must be changed, and there are suggestions which you believe are improvements. On the latter, it’s okay to think otherwise. You should try and be explicit in the case of each comment.
Your comments should be directed at the code, never the author. This may seem counterintuitive, but I assure you it’s of vital importance. The correct tone is the difference between a healthy discussion or making someone feel diminished.
What to look for
I’ve already talked about the where and the why, but not the how. You know you need to read the code and comment. But what should you be looking for within the code? The basis for a pull request is the difference between two commits. So, what happened between those two commits?
The intent of the changes
Stating the obvious: changes are made for a reason. If your project’s work is split into tasks, there was a task at hand. To understand the issue your colleague was tackling, you can read the task given before reading the implemented solution.
Common pitfall: do not review a PR with the author
When you don’t understand what a part of the new code does, what do you do? You ask the author. And then he or she explains this part and then the rest. Now you’re reviewing the code with the author. You will inherit the author’s biases. Everything you read is within the lens of the author’s view, making you understand things you will not understand next time you read them unless you remember the explanation. And you might even agree with things you would not to, due to context.
My general advice is to avoid reviewing code with the author. This is controversial, but I believe the benefits outweigh the disadvantages, and it’s more often than not a mistake.
Impact of the changes
Do the changes fit the application flow? Do they change the way something interacts with something else? Can something break?
When you were learning to program, you created mental models matching what code does. So, as a developer, you kind of can compile and run code in your head. You are able to reason about a flow. You can apply this mighty skill to code reviews. Run the code in your head and wonder if it goes wrong. It could be the changed code or other parts of the application.
Spot something fishy? Checkout the branch
Did you find something odd on the implementation and you’re not quite sure if it works? Checkout the branch and investigate a little bit more. Follow the flow within your editor of choice, and you will find either a mismatch in what you thought the code does or in what the code was supposed to do. So either a bug or more in-depth understanding. It’s a win-win. It is vital you understand each and every change you’re approving.
Spot bad practices
Code reviews are a great way to educate. If you spot anything you believe to be a bad practice, you can point it out. Something as simple as “One letter variables aren’t good because the name conveys no meaning” points out both the mistake and the reason why it’s a mistake. Pointing out again: PRs are a place for discussion. So prepare yourself for some amicable arguing.
Readability matters
Code is read way more often than it is written or modified. So you should, within reasonable limits, optimize for readability, because that is where you and your teammates will spend most of your time.
If you don’t think a variable name accurately represents what’s in it, suggest a better one.
Navigating big PRs
Most PRs should not be big. Big changes happen sometimes, but they are not the most common. So if someone is consistently making PRs with lots of changes, chances are they’re not separating the multiple concerns they’re tackling into separate PRs.
Here are a couple rules of thumb you can use and advise others to do so.
Keep refactors in separate PRs
Tackle one issue per PR
So, when you have a massive PR to review, here are some things you can do to help you navigate the logic and understand faster:
Look into the individual commits
Look at the new tests
Reading first the files you know have relevant changes
Reviewing Tests
Tests are also code that needs to be maintained, code in need of reviewing. A lot of people dismiss tests when they’re reviewing code, but this can reduce the quality of your tests.
Think of the corner cases
Corner cases need to be tested. So think of the corner cases you would check. See if they’re covered within the test cases written.
Do the tests look like they would fail?
When you’re writing a test, it’s important to see it fail. So in a review, it’s important to look out for the same kind of mistake. Do the tests look like they would fail if the tested code is malfunctioning?
Reject any test that was made based on an output
Sometimes people get real data to test their code, and they don’t trim the data down to the most relevant parts. So what happens is, they use that data and assume that non-empty values are correct. You can spot these in high numbers or big lists.
It gets easier over time
Reviews are especially hard when you’re not used to them or not familiar with the affected codebase. But don’t get discouraged. Like pretty much anything else in software development, it’s a learnable skill.
Top comments (1)
Good stuff. I would add that tools like CodeStream allow you to do that from the IDE, with better context and access to code navigation features.