Making Code Reviews Great Again!!
Code reviews have several purposes, the most common of which is checking for bugs, and inconsistencies.
Code reviews have several purposes, the most common of which is checking for bugs, and inconsistencies before the code is shipped.
Code reviews can be leveraged for so much more. At Turing Technologies, the primary purpose of code reviews is to spread knowledge and ensure quality. At their simplest, code reviews are a quick scan of someone else’s changes with an approval to merge. At their most in-depth, they can involve pairing, reviewing code line by line, and multiple feedback cycles. It's purpose is teaching tool to help developers learn when and how to apply techniques to improve code quality, consistency, and maintainability. Through thoughtfully evaluating code on a recurring basis, developers have the opportunity to learn different and potentially better ways of coding.
If you’re a new engineer or if your work has previously not gone through extensive code reviews then the first big lesson is to disconnect your ego from the work. It is very common for us humans to attach our emotions to something we’ve built.
“Human nature is such that we want and need to be acknowledged for our successes, not just shown our faults. Because development is necessarily a creative work that developers pour their soul into, it often can be close to their hearts. This makes the need for praise even more critical.” Robert Bogue: Effective Code Reviews Without the Pain
“An amateur is defensive. The professional finds learning (and even, occasionally, being shown up) to be enjoyable; they like being challenged and humbled, and engage in education as an ongoing and endless process. (...)” — Ryan Holiday, Ego Is the Enemy
Before we moving forward with code reviews, a reviewer needs to set some aims when doing a code review.
It’s hard to remember everything to check in a pull/merge request, and constantly switching contexts might cause gaps in your review as well. For example, if you’re focusing on the readability of a change, you might miss a risk to security or performance. The checklist moves from general to specific checks because it’s important to focus on the high-level attributes first. It doesn’t make sense to offer a variable name suggestion if you’re also suggesting that an entire class or function be refactored.
Here are the items and prompts in our checklist. Feel free to use this as a reference, and consider drafting a custom checklist by yourself or with your team.
If the requirements document for the features contain acceptance criteria then it is usually straightforward to determine if the PR/MR meets the expected requirements. If not, here are some prompts to help you find the answer to this question.
If your team creates detailed design documents before building, this should be an easy check. However, sometimes features become more complex than anticipated, and the design gets mixed with implementation. In these cases, it’s important to take a step back and ensure that you agree with the architecture.
At Turing Technologies, we usually work in monolithic applications (why we didn’t conform to the micro-services hype), so seemingly innocent changes can have unintended side effects on core functionality. If your system is less complex , this may be less important.
This check shouldn’t be green just because a test file exists. Review the tests. There is nothing worse than false positive tests.
The importance of this section depends on the type of work you do. We don’t handle millions of requests per second, but we do need to be mindful of performance when loading dozens of resources and relationships. Work with your team to determine how often you need to consider performance.
Our code needs to be simple and easily understood. As a result, we place an emphasis on this area, regardless of the size of the PR/MR.
Code formatting issues should almost never come up during a code review. Code reviews should provoke productive thoughts and discussions, and spending too much time on code style and formatting won’t help with that. Use tools such as linters and code formatters instead. Use linters to find code formatting and code-quality issues before a reviewer even gets involved. Whenever you see a repetitive comment during the review process, look for ways to minimize it (being with better guidelines or code automatization).
Code reviews are an opportunity for learning just as much as they are an opportunity to ensure quality in your codebase. Usually the best way to teach in code reviews is to simply leave longer and more informative comments. Add links to documentation and articles that help explain a concept or an alternative approach.
At the core of a code review, you’re providing feedback to your peers, which might be hard. But receiving feedback is harder. Everyone on your team is trying to do their best work, so take care in delivering your message. For example, if you’re pointing out an error or asking a question, make it a team effort, not their fault. This might look like: “Can we remove some of the duplication in this file?” instead of “You missed an edge case”.
Don’t forget to leave positive feedback in code reviews, too. If you appreciate the documentation in the pull request or learned something from the code, leave a comment and let the author know! Acknowledge great work and thank the author for their time and attention to detail.