Code reviews are a fairly standard part of the software development process at many companies. And for good reason – they are an excellent opportunity to catch design issues, silly mistakes, misunderstandings, or other general code issues. More eyes on a piece of code means a higher possibility of catching bugs.
Pull requests are commonly used as a vehicle for performing code reviews as it’s a great opportunity in the source code control workflow to perform the review. If you use github, bitbucket or a similar tool, you can generate a pull request with the push of a button. These tools also offer nifty UI diffs of the request compared against the mainline or target branch of where the changes are being merged to, making it very pleasant to read.
While the primary intent of this is for others to review the code, I highly recommend you review your own pull requests too.
Why, you may ask would you review your own code? After all, you wrote it. You already know what it does and how it works. And further, aren’t you totally biased and unable to objectively judge your own work?
Conversely, you may be thinking: “of course I double-check on my own work!, this goes without saying”. But there are various ways to go about doing that, and I think this one is particularly effective.
This subject has been written about elsewhere, but the posts i’ve come across have mainly focused on some of the obvious benefits like catching mistakes such as unintended commits, or commented out code. Of course these reasons alone should be enough to get you to review your own PR’s. But there are some more compelling reasons.
Here are a few of the key points & reasoning around why you should review your own PR’s:
Reviewing your code at least one day after completion
This does provide some amount of objectivity as you will have moved onto other things, changed your train of thought, context-switched a few times, etc.
Just like writing an important email, you might draft it the day before, have someone else read it over, but then read it over yourself one last time before you send it to catch any grammatical errors, adjust tone, make tweaks, etc. We’ve all heard the sage advice of never sending an email when you’re angry. You always want to approach it with a cool head, detached from the situation, so that you have a more balanced response. I think the same idea applies to code. Distancing yourself from the original thinking you had while writing the code can help you be somewhat more objective in reviewing it.
Comment Your PR
This is huge. I cannot understate how important this can be. I can’t count the number of times I started writing a comment for a part of a PR I generated and as I was typing it, I found an error or improvement that could be made which was directly related to the comment. It’s kind of like speaking out loud while problem solving in a way, or teaching someone else something you just learned – a piece of code you wrote can really sink in when you are trying to explain how it works to someone else. This can often lead to mental clarity that was not attainable when you were in the weeds while actally coding it.
If you follow the advice from the first point and review your PR at least one day later, commenting the PR can jog your memory and force you to explain what your changes accomplish and how they work. This can often uncover faulty logic mistakes.
Equally as important, is helping your reviewers understand your code. The more your reviewers understand what your code is supposed to do, the better they can assess whether it actually accomplishes the goal, or whether it can be improved. You should help facilitate this as much possible.
Now, you may argue that well-written code should be self-documenting, and I would very much agree. But a PR is a little more nuanced than a just a piece of code. It is more frequently a diff between the current code and your changes. As such, it can be very helpful to indicate why a change is being made, and potentially how it is correlated with another change within the PR.
Be your own worst critic
This is a good thing in code reviews. You often catch things that were very deep and someone else may be unlikely to catch. Remember, other reviewers are only human; they may not look over everything with a fine tooth comb. Let’s be honest, we’ve all glossed over someone else’s PR at one time or another. We’re all busy, and may have lots of other PR’s to go through. Which actually leads to the next point..
You may realize your PR is too big
Big PR’s can lead to the problems mentioned in the previous point. As a reviewer, it’s natural to have a tendency to gloss over a BEHEMOTH PR and just try to find the main points of interest. Help your reviewers be able to be more detail-oriented by keeping the PR’s a manageable size and break them up into several logical PR’s when they get too big.
One approach I’ve used with success in combating this problem is creating a PR with no assigned reviewers (this is possible in bitbucket, not sure about others) and no intention to merge it. This might seem odd, but the idea is that you can make a large change fearlessly, then go back through it in a PR, dissecting the essence of the changes and trying to distill them into separate and coherent PR’s. It’s an exploratory process intended to help you identify an incremental approach to your change, which may just not have been apparent until you got into the weeds. This can work really well on seemingly intractable re-factors.
Conclusion
Be kind to your reviewers and comment your PR’s. Help them help you find the errors or issues in your code by drawing attention to key areas, or make clarifications, etc. Help yourself learn and assess the work you did after the fact, in order to get better insight into what your change is about and how it might be able to be improved.
Stumbled on this book this week, and devoured it in an afternoon.