Have you ever found yourself in a situation where you needed to do a code review but didn't fully understand the project? Maybe you did not review it to avoid looking like you didn't know what you were doing.
This article assures you that there's a better way. You don't need to know everything to provide a code review. In fact, based on my experience, that's quite common.
I remember when I joined Red Hat as an intern and was asked to help with code reviews. We used a system of +1 or -1 votes, and I was initially very hesitant to weigh in. I found myself asking whether when I gave a +1 on a change but then someone else voted -1, would I look foolish?
What does happen if someone votes -1 on a change you've vote +1? The answer is nothing! You might have missed a detail that the other person noticed. It's not the end of the world. That's why we have this voting system. Like the rest of open source, merging code is a collaborative effort.
Lately, I've been so inundated with code reviews that I can hardly keep up with them. I also noticed that the number of contributors doing these reviews steadily decreased.
For this reason, I'm writing about my point of view on writing a code review. In this article, I'll share some helpful tips and tricks. I'll show you a few questions you should ask yourself and a few ideas of what to look for when doing a code review.
What is the purpose of a code review?
Have you ever written a really simple patch? Something you think is so trivial that it doesn't require a review? Maybe you merged it straight away. Later, it turns out there was a mistake, something obvious or silly, like the wrong indentation or a few duplicated lines of code instead of a function call (yes, I'm speaking from experience!).
A code review by someone else would have caught these things.
The point of a code review is to bring a fresh pair of eyes with a new perspective on the problem you're trying to solve. That new context is exactly the reason a code review is crucial.
You may think that you must be an expert in the language to review someone else's code, the project, or both. Here's a secret all code reviewers want you to know: That's wrong! You don't need to fully understand the project or the language to provide a fresh perspective on a change. There's a universal process of code review.
The universal process of a code review
Here's my process for code review, grouped into a couple of points. The process provides questions I ask myself to help me focus on a code change and its consequences. You don't need to go in this specific order. If there's a step, you can't execute for any reason, just move to another step.
1. Understand the change, what it's trying to solve, and why
The explanation of why the change is needed and any relevant context should be in the commit message. If it isn't, request it and feel free to -1 until it's provided.
Is it something that needs to be solved? Is it something the project should focus on, or is it completely out of scope?
2. How would you implement the solution? Would it be different?
At this point, you know what the code change is about. How would you have done it? Think about this before reviewing the change in detail. If the solution you have in mind is different from the one you're reviewing, and you think it's better, bring that up in the review. You don't need to -1 it; just ask why the author didn't go in this direction and see how the discussion evolves.
3. Run the code with and without the change
I usually put a few breakpoints into the code, run it, and inspect how the new code interacts with the rest.
If you can't run the whole code, try to copy the function containing the new code to a new local file, simulate the input data, and run that. This is helpful when you either don't know how to run the whole project or when it requires a specific environment to which you don't have access.
4. Can the new code break anything?
I mean, really anything. Think about the consequences.
In the case of a new command-line option, will it always be accepted by the target?
Can a situation occur when the option wouldn't be accepted or when it could conflict with something?
Maybe it's a new import. Is the new library, and possibly a new dependency, available in the older releases or systems you ship the project for?
What about security? Is the new dependency safe to use? The least you can do is run a quick Internet search to find out. Also, look for warnings in the console log. Sometimes there are more secure methods within the same library.
5. Is the code effective?
You've determined that the proposed solution is probably correct. Now it's time to check the code itself, its effectiveness, and its necessity.
Check the style of the new code. Does it match the style of the project? Any open source project has (or should have) a document informing (new) contributors about the styles and good practices the project follows.
For instance, every project in the OpenStack community has a HACKING.rst file. There's often also a guide for new contributors with all the must-know information.
6. Check that all new variables and imports are used
Often, there have been many iterations of the code you're reviewing, and sometimes the final version is very different from when it started. It's easy to forget an import or a new variable that was needed in a former version of the new code. Automation usually checks these things using linting tools like flake8 in the case of Python code.
Can you rewrite the code without declaring new variables? Well, usually, yes, but the question is whether it's better that way. Does it bring any benefit? The goal isn't to create as many one-liners as possible. The goal is to write code that is both efficient and easy to read.
7. Are the new functions or methods necessary?
Is there a similar function that can be reused somewhere in the project? It's always worth helping to avoid reinventing the wheel and re-implementing logic that's already been defined.
8. Are there unit tests?
If the patch adds a new function or new logic in a function, it should also include new unit tests for that. It's always better when the author of a new function also writes unit tests for it.
9. Verify refactoring
If the commit refactors existing code (it renames a variable, changes variable scope, changes the footprint of a function by adding or removing arguments, or removes something), ask yourself:
- Can this be removed? Will it affect the stable branch?
- Are all the occurrences deleted?
You can use the grep command to find out. You wouldn't believe how many times I've voted -1 just because of this. This is a simple mistake that anyone can make, but that also means anyone can uncover it.
The owner of the commit can easily overlook these things, which is totally understandable. It's happened to me many times too. I'd finally figured out the root of the problem I'd been fixing, so I was in a rush to propose the review, and then I forgot to check the whole repo.
Apart from the project's repository, sometimes it's also necessary to check other code consumers. If some other project imports this one, they may need refactoring, too. In the OpenStack community, we have a tool that searches across every community project.
10. Does project documentation need to be modified?
Again, you can use the grep command to check whether the project documentation mentions anything related to the code change. Apply common sense to determine whether a change needs to be documented for end users or it's just an internal change that doesn't affect user experience.
Bonus tip: Be considerate
Be considerate, precise, and descriptive if you make a suggestion or comment on something after you've reviewed the new code. Ask questions if you don't understand something. If you think the code is wrong, explain why you think so. Remember, the author can't fix it if they don't know what's broken.
Final words
The only bad review is no review. By reviewing and voting, you provide your point of view and vote only for that. Nobody expects you to give the final yes or no (unless you're a core maintainer!), but the voting system allows you to provide your perspective and opinion. A patch owner will be glad you did it, trust me.
Can you think of any other steps for a good review? Do you have any special technique different from mine? Let us all know in the comments!
Comments are closed.