I Review My Own Pull Requests
Sometimes I even add comments — it’s the polite thing to do
I review all my own pull requests. Some more thoroughly than others, but I try to make it part of my process for every change. Why? It helps me make my changes easier for other people to review. It also helps me make sure I’m preserving the repository’s existing commit history, where I can.
The review starts with a quick scan through the changed files. I find it really helpful seeing the whole diff laid out on a single web page. If I’ve made several commits or been working on the code for a day or two, I don’t necessarily have a clear picture of which lines and files I’ve changed. The web UI makes that really clear. Not only that, but this is the way that other reviewers are going to see the code for the first time. I want to make sure I’m giving them something presentable.
One of the first things I’m hunting for is files that don’t need to have changed at all. Reducing the number of files in a pull request is one of the most impactful ways to make it easier to review. Did my IDE helpfully reorganize some imports in a file, or add a new line at the end? That’s great, but only if I actually ended up committing some other code changes in that file. If not, those files don’t belong in this PR, and the changes can wait for another time.
Next up, are there any lines where the only thing that’s changed is formatting or whitespace? Those kinds of changes can sneak in easily when I’m trying out different solutions to a problem. If I go near a line of code, my IDE will reformat it automatically. But once I’ve settled on an approach, I want to go back and eliminate any extraneous formatting changes that I made along the way. My preferences aren’t important here. Maybe that function signature would look better with a line break or two in it, but the PR would look even better if that function signature didn’t change at all.
I even try to get rid of formatting changes where the old code was technically “wrong” by our team’s code style. One of the main reasons a consistent code style is useful is that it helps reduce churn and extraneous details in pull requests. Yes, code that’s consistent is easier to read and review. But if the old code didn’t match the agreed style in the first place, changing extra lines just to “fix” their formatting is making the PR harder to review, not easier. If I’m changing the line for another reason, of course, I’ll fix its formatting too, but otherwise, I’m reverting the change.
Indentation is one area where I like to get a little creative. In a language like Kotlin, it can happen quite often that a refactoring change will affect the nesting level of a block of code by adding or removing some braces. If I can counterattack by swapping a block function for an expression function, or by adding some extra optional braces to a when statement, I’ll do my best to make the new indentation match the old.
Of course, when that’s not possible, the “Hide whitespace changes” option in GitHub is an invaluable tool that all code reviewers should know about.
One thing that’s harder to pull off, but deeply satisfying, is when you can shorten the diff by putting things in a different order. Did the new function I added end up actually being similar to the one that I removed elsewhere in the file? Let’s make sure the new function is in the same place as the old one so that any lines in common can be elided. I have no strong feelings at all about how things should be ordered in a source file, and luckily my team’s code style doesn’t prescribe an order either. So I pick the order that makes for the best-looking diff. It isn’t just a cosmetic choice, though. For every line I can leave untouched, the commit history will be that much simpler, and my future self will thank me when I have to do some git blame detective work.
Why would I add comments to my own PR, though? Again, it’s all about making it easier for people to review. Even a small code change can be disorienting at first, and a big one is much worse. Adding a few comments lets me point out the interesting bits, or highlight particular areas where I’m looking for feedback and suggestions.
Do I sometimes take it too far? Maybe. Trying to minimize the number of lines changed in a PR can be addictive. I’ve been known to push a new commit just because I spotted a single character in the diff that didn’t need to be there. But I’m going to keep doing it, and hopefully, the colleagues reviewing my code will have a slightly better day because of it.
