Self Review
An easy thing that I think every developer should be doing is performing a review of their own code before making a pull request (PR) or merge request (MR) and having other people look at their code. It may sound a bit silly but I think there are a few benefits we get from doing this.
Benefits⌗
1. Catching silly issues⌗
Everyone has had that comment come in on their PR saying they accidentally left in a TODO or forgot to remove a bit of console logging they added while working on a change. This is generally easy to remove and doesn’t take much effort to deal with, it does require extra time on everyone’s part.
Your reviewer has to spend some time figuring out if this is intentional or not and then some extra time pointing it out. This gets especially tedious if there are a number of examples of this problem. You then have to spend time responding to the comment and working on a fix. Finally your CI pipeline then needs to rerun and that’s not free either! Finally the reviewer has to come back in and recheck the code again which could be pulling them out of the flow of what they were doing. While it shouldn’t take long to fix these can add up as measurable expenses and would be best avoided.
Going through your code before everyone else should hopefully catch these kinds of things before anyone else does.
2. Gives CI some time to catch issues⌗
This is a smaller win but still a win none the less. It’s annoying as a reviewer when you get pinged about a PR you need to review and you get there and the build is failing. Do you still review it knowing that a new change will be incoming?
Hopefully while you’re double checking your work, CI will have had enough time to finish running and if there’s an issue you can fix it before sending things off to reviewers.
3. You can verify the change is complete⌗
Depending on how your day is going or how many meetings you have you can’t always get a solid chunk of time to focus on the one feature you’re supposed to be working on and you have to start and stop a few times. Each of those context switches introduces a risk that some requirement or detail will get missed. We’re only human and mistakes happen and we get busy.
Taking a moment to double check that everything you expect to be there is actually there can help catch these mistakes.
4. Prepare comments explaining tricky bits⌗
Depending on the complexity of the change, there may have been decisions that were made along the way that could be likely to invite feedback from reviewers. You should add code comments for those areas or else have a PR comment prepared to explain why those choices were made. Hopefully that will completely remove the need for reviewers to comment but even if it doesn’t it might make the conversation a little shorter since you’ve already skipped some number of steps ahead.
You might have also had to write some more challenging code that could be harder to reason about or needed to be written in a very specific way to work as intended. Call that out in code comments. Again this shortens how much time needs to be spent on the back and forth between reviewers and the reviewee. It’ll also make things easier for some future reader who needs to understand this code.
One quick example of what I mean here would be some Typescript code like this:
type Config = {
coolFeatureEnabled?: boolean;
};
function isCoolFeatureEnabled(config: Config) {
return config.coolFeatureEnabled !== false;
}
A reader might see that and immediately ask something like this:
Why not just do something like
return Boolean(config.coolFeatureEnabled)
Totally get where that reviewer is coming from however them meaning between the two isn’t actually the same. Here’s a quick table showing the behavior difference between the two versions:
Config Value | Initial Version | Reviewer Version |
---|---|---|
true |
✅ | ✅ |
false |
❌ | ❌ |
undefined |
✅ | ❌ |
That undefined
case changed between the two versions in this case
that could be a problem if we interpret undefined
as “give me the
default”. A quick comment would have alleviated the issue.
type Config = {
coolFeatureEnabled?: boolean;
};
function isCoolFeatureEnabled(config: Config) {
// Now that the feature is considered "mature" we are switching to
// it being generally available and on by default.
// Since `undefined` means give me the default we need to explicitly
// check if the value is false and not falsy.
return config.coolFeatureEnabled !== false;
}
5. Providing a better experience for other reviewers⌗
If you don’t feel like reviewing your own change then that may be an indicator that there’s something with it that will make other people not really want to review it. A quick example of this would be if the change results in a really big PR. If the PR is so big you don’t want to review it then it’s reasonable to assume that everyone else will think it’s too big and not want to review it.
Reviewing your own code creates sympathy with your reviewers.
How to do it⌗
Hopefully you’re convinced at this point that you should be trying to review your own changes before asking others and are now wondering how to go about that.
There are three ways that I’ve done this in the past and I choose based on the change or by how noisy creating a PR is.
git diff
locally⌗
You can run git diff
before making your PR if you want to do things
completely local to your machine.
If you plan to make a single commit and PR of all your changes you
can do git diff
before you commit your changes.
If you’ve already staged your code but haven’t committed it then you
can do git diff --staged
.
If you’ve committed your changes or have multiple commits that you
plan to make a PR with you can diff it against a parent branch.
Assuming that main
is the parent branch you’ll be merging post PR to
then you can do git diff main
.
I highly recommend using difftastic
as it provides super powers for git diff
.
PR preview⌗
Most of the hosted source control systems that I’ve used (Github, Bitbucket, Gitlab, Gitea, etc) provide a way to preview your changes before completing the creation of your PR. Some of them even let you add comments at this stage.
Also depending on your CI setup, because you’ve already pushed your code, CI will be running and might finish before you actually hit the create button the PR.
Private PR⌗
The final option that I use is to create the PR but not actually add any reviewers. I can review the code, add PR comments, and make sure the wording on the PR description makes sense. CI can also have a moment to run if it hasn’t finished already.