Code Review

Overview

Although we pair program occasionally, most code at Wunder is written by a single author. Before merging and deploying, all code is asynchronously reviewed as a pull request on GitHub.

The Benefits of Code Review

Through code review, reviewers can provide feedback to authors:

  • Reviewers catch bugs
  • Reviewers suggest ways to improve the clarity of the code
  • Authors can ask questions about trade-offs and get the reviewer’s opinion
  • Reviewers can alert authors to libraries and techniques that the author may not be aware of

However, the most important reason we review code is to radiate knowledge among the team. After a review, both the author and reviewer have a deep understanding of the feature. This means:

  • If any bugs appear in production, the author can collaborate with the reviewer to quickly fix the bugs
  • The reviewer can do additional work on the feature as an author
  • The author can go on vacation or work on another project, since the reviewer can provide support for the feature

Forward Progress

To ensure that code review doesn’t slow down our progress, we rely on two Wunder Operating Principles.

Since we value speed, the author chooses and notifies a single reviewer. Reviewers complete reviews quickly (usually within a business day or two, depending on urgency).

Since we believe the person closest to the problem should solve the problem, the author makes the final call on all decisions in a pull request. Reviewers should suggest improvements, point out errors, and explain any disagreement, but the author always has the final say.

Choosing a Reviewer

When choosing a reviewer, we emphasize knowledge sharing rather than reinforcing existing expertise. This limits knowledge silos and spreads the load of reviews across the team.

In practice this means:

  • We should avoid always assigning reviews to an “expert” in a given topic. When in doubt, randomly assign a reviewer.
  • Reviews from a “non-expert” may take longer but share more knowledge. That’s often a good trade-off.
  • Authors must own the production-readiness of their code without relying on an approval from a specific person.

“Good Enough for Today” (GEFT)

Engineering is all about making trade-offs and each engineer has a unique history and perspective that informs the way they solve problems.

We solicit feedback in reviews, but occasionally the author needs to move forward with a solution that other engineers disagree with. Often this is because we just don’t have enough data yet to decide between two approaches, so we prefer to ship and revisit when we know more.

This happens commonly enough that we’ve adopted shorthand for it: Good Enough for Today (GEFT). It’s a very succinct way for an author to tell a reviewer:

  1. I appreciate your concerns
  2. I’ve decided to keep the current solution (perhaps because I think the my solution is better, or because I don’t have enough data to decide, or because I think the effort of changing the solution isn’t warranted)
  3. I’m going to ship
  4. Let’s revisit later if my solution causes problems