Code review as a tool to improve quality. Part 1 – how to provide feedback.

Other posts in this series

Part 2 – what to look out for in code reviews
Part 3 – why should you care?

Reviewing code is a fairly standard practice in most software teams.
However, like with any deeply ingrained practice that we take for granted, the rationale and purpose behind the practice can get lost, and the process is only followed mechanically, but not effectively (did someone say agile ceremonies?).
In this series of posts, I’ll take a look at what makes for an effective code review process. And furthermore – how I believe it can be leveraged to increase the quality of code for the team.

But first we must begin with a –

Definition

A code review (or peer review) is the process whereby proposed changes to a codebase by an author are inspected by a reviewer.
The mechanics of how this is done can vary – Github has popularized the “pull request” concept, but reviews can also occur over email, or even by just grabbing a colleague to come view your code.

Goals

To make a process effective, we should first define what it is we’re trying to accomplish. Here are the goals of a code review as I see them:

1. Quality control
   This is about ensuring a standard of code quality – not about finding bugs or mistakes.
   (Although, if we review test code as part of the proposed change, then this might serve for that purpose as well.)
   The reviewer tries to ensure that the proposed changes meet certain bars of quality. If it doesn’t, they may suggest ways to improve the code’s quality.
   (“Code quality” can be subjective, and I’ll try to define what (I think) it means in the next posts.)
2. Knowledge sharing: Reviewer –> Author
   The reviewer can suggest simpler, more efficient or more maintainable options for implementing the proposed changes.
   This may be because they’re familiar with a pattern or a language feature that the author is not aware of. Or they may know that someone has already implemented similar code that the author could re-use.
3. Knowledge sharing: Author –> Reviewer
   Firstly, like the above – it’s possible that the author has used a pattern / language feature that the reviewer is not familiar with. The reviewer has now learned something new.
   Secondly, this is an opportunity for the reviewer to familiarize themselves with a different area of the codebase. This means that, in the future, the reviewer will be able to more easily maintain this code.
4. Knowledge sharing: Documentation
   In some cases, the review process is done in writing (like a pull request). In those cases, the review itself becomes an artefact that can later be referred to.
   For example – when a maintainer wants to understand the purpose of a certain piece of code, or why it was written in a certain way.

Some important corollaries of the above goals are –

1. A review is a communication method for technical leadership. By accepting or rejecting code, they signal to the team what kind of code is and is not acceptable.
2. A review is a mentoring opportunity. It’s a chance for the reviewer to teach the author something that they did not already know (or learn from them).

Review as a mentoring tool

This is the first area I’d like to touch – how to make a review an effective tool for teaching and learning.

1. Be kind

Developers are only humans, and are subject to human reactions. We are much less likely to take advice or feedback on board if we feel attacked or insulted.
Phrase your feedback in an encouraging and constructive way.
For example, comments such as “I can’t merge this – this change will break the entire app!” or “This code is a bit messy – Please tidy this up before merging” are both antagonizing and unhelpful.
Even if those statements are factually correct, they don’t contribute anything to solving the issue.
Comments like “Looks like these changes break the signup functionality in X scenario; could you make sure it’s fixed before merging?” or “I find it hard to wrap my head around the implementation of the email reminder functionality; could it be simplified?” are much more helpful as a first step to resolve the issue (and actually point the author to what the issue is).

2. Teach a woman to fish

As we discussed, one of the goals of a code review is knowledge sharing. Meaning – teaching the author something they don’t yet know.
Take these opportunities to teach. Don’t focus only on “fixing” the immediate problem at hand.

Firstly, explain why you’ve made your comment, and what general principle or pattern you have in mind.
For example, rather than “This class should be smaller”, explain that “This class has too many responsibilities”.

Secondly, prefer to not prescribe a solution to an issue you’ve found.
This is because we learn better when we figure out something for ourselves, rather than being instructed.
Also, force-feeding solutions might serve to demotivate your colleagues. They may feel as though you’re treating them like code monkeys, not allowing them to think for themselves.

My preferred approach is to identify the issue clearly, and prompt the author to find a solution.
Examples include “This method seems a bit long; would it be possible to shorten it?”, or “Looks like this code is being repeated in both the DBConfiguration and XmlConfiguration classes; could we DRY it up?”

There may be cases where you do choose to suggest a solution. Make sure to explain why you’re suggesting a certain change, and what general principle or pattern you have in mind.
For example, rather than simply instructing the author to “Remove the duplication in the DBConfiguration and XmlConfiguration classes by extracting everything except the actual data fetching into a base class, which defines a virtual getConfig method that will be implemented differently by both subclasses” , explain what the “template method” design pattern is, and when it’s appropriate to apply it.

3. Seek to learn

Ask questions to make sure that you have a good understanding of the proposed changes. Ask the author why they chose a certain implementation over another. Ask about anything that seem non-standard, or that you haven’t seen before. Ask if there’s anything you don’t understand. Asking for clarifications achieves multiple goals –

• It verifies your understanding of the code (meaning – you’ll be able to more easily maintain it in the future)
• It challenges the author to re-think and justify their choices – They may find a better alternative in the process.
• It helps you learn from the code.

Conclusion

Remember that a code review is not just about this particular change.
A review is one of the easiest, lowest-cost teaching opportunities you can get.
Approach your review as starting a conversation rather than a binary approve / reject scenario.
Many times I’ve found that I, as the reviewer, have learned something new as a result of that conversation.

Read next

Part 2 – what to look out for in code reviews
Part 3 – why should you care?