Making Code Reviews Great Again!!

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.

1. Foster a friendly environment. Use code reviews to deliberately foster a positive, patient and friendly work environment.
2. Improve your working relationships by having a chance to talk to your fellow developers.
3. Relieve tension by giving people positive feedback.
4. Learn. A reviewer can learn new things from the code they’re reviewing, and an author can learn from the feedback they receive.
5. Mentoring and education. Code review is a chance for more experienced developers to mentor and teach others. But be aware that code review, after time has already been spent and code written, isn’t always the best time for mentoring and education. Developers should already have collaborated on the technical design before and during the code writing itself.
6. Code quality. Finally, yes, code quality (including bugs, maintainability, documentation, organization, architecture, usability, …) is one of the aims of code review. Code review can be a time to get a fresh perspective on your approach, code or thinking. 

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.

1. Meets Requirements

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.

• Does this PR/MR satisfy the expected requirements?
• Does the proposed UI match given mockups and designs?
• Does the proposed change fit within the scope of the ticket?
  

2. Architecture & Design

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.

• How was this feature implemented? Can I easily discern the design from the code?
• Does it follow an agreed upon design? If not, does the structure of the implementation make sense?
• Is it simple or over-engineered?
• Is the code secure (e.g. no SQL injections, etc.) - https://owasp.org/www-project-top-ten/
• Is the code and the actions it performs observable? Can we track actions using metrics, logging or tracing?
• Is the code open to extension in the future? Is it specific or generic? Have abstractions been made too early?
• If a new engineer joined your team tomorrow, could they understand this implementation?
  
  

3. Interactions & Side Effects

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.

• Is it possible that these changes have unintended effects in other parts of the system?
• If an existing function was changed, were all usages updated?
• If external dependencies were added, have they been evaluated?
  

4. Test Coverage

This check shouldn’t be green just because a test file exists. Review the tests. There is nothing worse than false positive tests.

• Are there tests? Should there be tests?
• Do the added tests have the potential to flake in the future?
• Is the correct functionality being tested? Are the tests structured appropriately?
• Is it using unit tests where possible, integration tests where necessary?
• Are edge cases accounted for and being tested? Are there edge cases that aren't being handled?
  

5. Performance

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.

• Has optimization been properly balanced with readability?
• Is there a potential performance bottleneck?
• What queries are being made? Are they retrieving more data than is being used?
• Does the proposed change have the potential to affect performance in other areas of the system?
  

6. Readability & Style

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.

• Can you understand the code without explanations from the author?
• Could you debug this code without the author’s help? Could you extend the code in the future without the author’s help?
• Is it clear what the purpose of a variable is from the name? Is it clear what the intent of a function is from the name? Is it clear what the expected type of an object should be?

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).

‍Other things to consider:‍

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.