Effective code review with two simple rules
Code reviews are quickly becoming an industry standard practice for improving software quality. Open source workflows slowly become business best practices as businesses start to appreciate the costs of technical debt and become more concerned with software quality. Over the past decade, the absence of unit testing in the workplace eventually turned into a required developer skill. The days of submitting our first draft as a finished piece of work are gone: peer code reviews are the next developer core competency for software quality, inherited from social coding features of popular open source coding platforms like GitHub.
GitHub released its formal code review workflow just a few months ago, but informally, code reviews have been a foundation of the platform for years thanks to features like pull requests and inline comments. Its competitors, such as BitBucket and GitLab, share similar features. Code review tools also exist for self-managed solutions, like JIRA plug-in, Crucible.
Code reviews mean we no longer work by ourselves: coding is a social process. Even if we write code by ourselves, at least one other person will check it before it is committed to a shared repository. This has the potential to create conflicts between developers as two or more persons with strong opinions inevitably collide. This is something we're all susceptible to: sometimes we fail to remain humble or objective in the face of criticism of our work, but it is not just the reviewee's fault when conflicts occur; sometimes the reviewer hasn't done a good enough job writing the review.
Writing effective code reviews
How do we write good code reviews? A detailed guide to code reviews has been published by thoughtbot and even adopted by GitLab, and it's a great read, but this article proposes effective code review with just two simple rules.
- Identify a problem.
- Propose a solution.
For every comment we make as a reviewer we must ensure both rules are followed: it is insufficient to do just one of these things. But all too often, a reviewer will leave a comment that does just one, which is often why conflicts and frustration occur between author and reviewer. Let's look at each rule in more detail to see why both are important, but by themselves are insufficient.
Identify a problem
Identifying a problem is crucial to establishing that a change is necessary. If there is no problem, why should the author feel compelled to make a change? Offer objective evidence that establishes a problem really exists and thus incentivise the author to make a change.
Sometimes we identify a problem but don't have a solution to offer. It is not strictly incorrect to leave such a comment on a review, but such comments often offer little value because if we don't have a solution there's nothing to suggest the author does either. If we're lucky, someone else might have a solution, but it is incorrect to require the author to make a change based on a problem without a solution because we wouldn't be able to do a better job ourselves.
Propose a solution
Take the time to propose at least one solution to the problem. Nothing motivates the author to revisit their code to make a change like seeing a better solution well presented. If we can propose several alternatives, all better than the original, this empowers the author to choose the one they prefer, further encouraging them to make a change. Don't be afraid to write code in a review: someone has to write it, and there's no reason why the original author has to do all the work, because social coding is a collaborative process through which the best results can only be delivered by working together.
However, beware of proposing solutions without establishing the existence of a problem! This is the most common mistake made by inexperienced reviewers whom state, do it this way instead, without establishing why. If we cannot explain why our solution is better than the current implementation the author is not compelled to make a change. Sometimes our instincts are correct, our solution is better, but we must develop the skill to objectively express why our approach is better. Never assume it is obvious to the reader that we are correct without offering proof.
Sometimes our solution is not better, it is just different. Sometimes, through trying to prove our solution is better, we realize even before publishing our comment that it isn't better, it's just an alternative. In this case we're just inflicting our personal preferences on others; this is why it's paramount to identify the problem before proposing a solution.
Before submitting code for review
The focus of this article is on writing code reviews, but it would be remiss not to mention the author's responsibility when submitting code for review. When we submit code there is just one simple rule to follow.
- Never submit the first draft.
The first person to review our work must always be ourselves. It places an unfair burden on the reviewer and wastes their time to spot mistakes we could have found ourselves. Every problem the reviewer identifies in our work should teach us something new; if it does not, we didn't do a good enough job checking our work before submission.
Summary
Code reviews are becoming commonplace both with collaborators in online communities and with colleagues in the workplace. By following two simple rules we can ensure we write better code reviews, reducing the chance of conflicts and frustration through miscommunication, and insodoing gain the respect and admiration of our peers for helping them write better code together with us.
And remember: don't be too hard on others, or yourself; we all make mistakes.
