Review driven development?

We've heard of test-driven development, behaviour-driven development, feature-driven development and someone has probably invented buzzword-driven development by now. Here's my own new buzzword phrase: review-driven development. At ComputerMinds, we aim to put our work through peer reviews to ensure quality and to share knowledge around the team. Chris has recently written about why and how we review our work. We took some time on our last team 'CMDay' to discuss how we could make doing peer reviews better. We found ourselves answering this question:

Why is reviewing hard? How can we make it easier?

We had recently run into a couple of specific scenarios that had triggered our discussion. For one, pressure to complete the work had meant reviews were rushed or incomplete. The other scenario had involved such a large set of code changes that reviewing them all at the end was almost impossible. I'm glad of the opportunity to reflect on our practice. Here are some of the points we have come away with - please add your own thoughts in the comments section below.

1. Coders, help your reviewers

The person that does the development work is the ideal person to make a review easy. The description field of a pull request can be used to write a summary of changes, and to show where the reviewer should start. They can provide links back to the ticket(s) in the project's issue tracking system (e.g. Redmine/Jira), and maybe copy across any relevant acceptance criteria. The coder can chase a colleague to get a review, and then chase them up to continue discussions, as it is inevitable that reviewers will have questions.

2. Small reviews are easier

Complicated changes may be just as daunting to review as to build. So break them up into smaller chunks that can be reviewed easier. This has the massive benefit of forcing a developer to really understand what they're doing. A divide & conquer approach can make for a better implementation and is often easier to maintain too, so the benefits aren't only felt by reviewers.

3. Review early & often

Some changes can get pretty big over time. They may not be easy to break up into separate chunks, but work on them could well be broken up into iterations, building on top of each other. Early iterations may be full of holes or @TODO comments, but they still reveal much about the developer's intentions & understanding. So the review process can start as early as the planning stage, even when there's no code to actually review. Then as the changes to code take shape, the developer can continually return to the same person every so often. They will have contextual knowledge growing as the changes grow, to understand what's going on, helping them provide a better review.

4. Anyone can review

Inevitably some colleagues are more experienced than others - but we believe reviews are best shared around. Whether talking about your own code, or understanding someone else's code, experience is spread across the team. Fresh eyes are sometimes all that's needed to spot issues. Other times, it's merely the act of putting your own work up for scrutiny that forces you to get things right.

5. Reviewers, be proactive!

Developers like to be working, not waiting for feedback. Once they've got someone to agree to review their work, they have probably moved onto solving their next problem. However well they may have written up their work, it's best for the reviewer to chase the developer and talk through the work, ideally face-to-face. Even if the reviewer then goes away to test the changes, or there's another delay, it's best for the reviewer to be as proactive as possible. Clarify as much as needed. Chase down the answers. Ask seemingly dumb questions. Especially if you trust the developer - that probably means you can learn something from them too!

6. Use the tools well

Some code changes can be ignored or skipped through easily. Things like the boilerplate code around features exports in Drupal 7, or changes to composer.lock files. Pointers from the developer to the reviewer of what files/changes are important are really helpful. Reviewers themselves can also get wise as to what things to focus on. Tools can help with this - hiding whitespace changes in diffs, the files tab of PRs on github, or three-way merge tools, for example. Screenshots or videos are essential for communicating between developer & reviewer about visual elements when they can't meet face-to-face.

7. What can we learn from drupal.org?

The patch-based workflow that we are forced to use on drupal.org doesn't get a lot of good press. (I'm super excited for the gitlab integration that will change this!) But it has stood the test of time. There are lessons we can draw from our time spent in its issue queues and contributing patches to core and contrib projects. For example, patches often go through two types of review, which I'd call 'focussed nitpicks' and the wider 'approach critiques'. It can be too tempting to write code to only fulfil precise acceptance criteria, or to pass tests - but reviewers are humans, each with their own perspectives to anticipate. Aiming for helpful reviews can be even more useful for all involved in the long-run than merely aiming to resolve a ticket.

8. Enforcing reviews

We tailor our workflow for each client and project - different amounts of testing, project management and process are appropriate for each one. So 'review-driven development' isn't a strict policy to be enforced, but a way of thinking about our work. When it is helpful, we use Github's functionality to protect branches and require reviews or merges via pull requests. This helps us to transparently deliver quality code. We also find this workflow particularly handy because we can broadcast notifications in Slack of new pull requests or merges that will trigger automatic deployments.

What holds you back from doing reviews? What makes a review easier?

I've only touched on some the things we've discussed and there's bound to be even more that we haven't thought of. Let us know what you do to improve peer reviewing in the comments!