At all of my jobs to date, I’ve been fortunate to have someone on my team who has been a domain expert in the language or technology I’ve been working. In addition to this, I’ve worked on different stacks in each role. Ruby stacks in my first role, then iOS, and now the JavaScript world. In addition to my own learning I do on-task and outside of work, I find code review a really valuable learning experience.
However, in recent times, I’ve found myself making similar mistakes in my pull requests, or doing things I’ve already been asked to do differently. While learning something new, there’s a lot to remember and try do properly, and it turns out I couldn’t remember it all in my head. I found this embarrassing and it lead to a fair bit of frustration within me, not to mention the understandable disappointment from the domain expert trying to help me. After a bit of thought, I thought enough was enough and it was time for a solution.
Introducing Self-Review Checklists
I settled on what I call a “Self-Review Checklist”. It’s a list of things I’ll check as I’m doing my self-review of my code before sending it to the reviewer. It often finds a bunch of things to fix that occurred during the development process.
I created this list by spending about an hour and going through all the previous code reviews I’d received. I made a list of every comment I’d ever got that was generic enough to apply to other codebases or in future reviews. Here are some from my list:
Make sure you’ve got eslint on and working
Can you use .map instead of .forEach?
Put utilities in well-named files in the right directory
Check logging to the error reporting service sufficiently masks any sensitive data
Have you used “any” in TypeScript only when you cant type it thoroughly?
My list contains about 30 things now, and it usually grows after every review. I’ve organised my list by “general” and then into sub-sections per codebase I work on. Sometimes certain code bases have specific things to watch out for, or less automatic checking such as linting or perhaps uses JavaScript instead Typescript.
At the moment, these things are listed just to prompt my memory and don’t have examples or further explanation than required. I think this is okay because this list is meant for my personal use. I don’t intend to share it, nor do I think it should be some sort of checklist all developers on my team follow. These are things that I found non-obvious, or that I need to work on to make code that fits my team’s expectations.
How has it gone?
I’ve been using this list for about two months now and I’m proud to say that I think my code reviews have gone much better. I feel more general satisfaction from my reviewers, and they’re able to point out even more detailed things I can work on, or point out even new things I can learn from – and I really love that. I’ve even used this list with my reviews of other’s work too – and has made me a more effective reviewer.
I intend to remove some items when I become more confident that I’m in the habit, but for now I’m keeping the complete list. Am I making the same mistakes now though? Often not, which is great and signs that I’m proceeding through the Conscious Competence Learning Model.
I’ll still make mistakes - and that’s okay!
While the checklist has helped enormously with my code reviews. I have no doubt I’ll still make mistakes. And I think that is a completely reasonable thing to happen. If I didn’t, then code review would be pretty pointless wouldn't it? However, taking the time and having the discipline to thoroughly self-review eliminates a lot of high-level silly mistakes and lets the reviewer focus on the important parts of the review. As a reviewer, I am careful not to be quick to rage or abuse when a team member makes mistakes like this, it is just that - a mistake. They’re not out to annoy me or have any malice at all. I’ll leave a simple comment such as “left some commented out code here” or “oops, looks like this is still hardcoded?”.
However, my new expectation is that if I’m pointing out something for the second or third time for the same team member, then I will experience disappointment and frustration - and I’ll likely voice that. Similar to what I’ve done for myself, I’ll put the responsibility back onto the team member to correct. I love the code review process, and I’m passionate about making it better in every team I join. However, individuals also need to take ownership of their code reviews and make sure its a positive experience for them and the reviewer and make sure it’s effective as possible.
In conclusion, I thoroughly recommend self-review checklists as a tool if you’re experiencing similar issues at your work. Even if not, I think the act of reflecting thoroughly on code review feedback as I do now will further cement and accelerate your learning.