I've read a lot of posts at this point on how to write code, how to write good code, how to use your tools well (even attempted one of those), and so forth. But I've never seen good advice for how to receive code review, and will attempt to correct that here.

In a reasonably functioning project, successful incorporation of changes follows a predictable process. It's usually a familiar flow:

  1. Contributor proposes changes
  2. CI runs over said changes
  3. Contributor modifies their changes if necessary and repeats from (2)
  4. Some nonzero number of other contributors (possibly maintainers) provide feedback ("code review")
  5. Contributor modifies their changes if necessary and repeats from (4)
  6. Code becomes part of the project.

I draw a distinction between CI results and human-provided code review because CI results are not sufficient for code review! While neither can replace the other, good CI enforces that code works, while code review enforces that the code is good.

Thinking about process

From a maintainer's perspective, that's the primary role of code review: to ensure project quality and continued maintainability. But there's an important secondary purpose as well: to help contributors (and potential contributors) learn and grow. In other words, receiving code review is a learning and growth opportunity, and should be approached as such.

And so, first and foremost: code review is not a judgement on you. It's a second set of eyes, and both of you are trying to make sure the changes are good. If they didn't want the change in the project, they'd say so! Subtlety isn't what's happening here. And besides, if anyone were perfect, we would do code review.

Which leads into: everyone needs code review. No change is too small for it, and no one is perfect. I've broken builds by changing only documentation, and flagged potential security issues from developers who have been coding almost as long as I've been alive. (And they've done the same to me!) That's normal. That's life. That's code review.

And it's fine, because we don't need it to be perfect on the first try. Contributing to open source isn't a school exam where we get a single attempt and it's most of the grade. We're concerned only with improving our software, and if there's grading at all, it's externally imposed (e.g., by an employer).

Working with the process

But we have finite time, so we try to keep the number of round-trips to a minimum. Which works out well, because it incentivizes trying to get it right the first time - while knowing we probably won't. And that's a good place to be, really.

To make that a bit more concrete, here are some things I recommend doing before sending your changes for review:

  • Self-review. If you wouldn't merge the change, why would anyone else reasonable?

  • Document. Writing good comments is the subject for another post, but you need to have them - no decently sized codebase can reasonably survive without them.

  • Contain. Everything about what your code does and why should be contained within the code itself; everything contextual should be in the commit message. There probably shouldn't be an extensive "cover letter" (or pull request message, etc.) beyond that.

  • Test it. Just check it works before you wait on CI, if you can.

  • Get feedback early. If you're designing something complex, you don't want to waste time building something that'll need to be rebuilt later.

Writing good code is a skill, not a contest. We all improve with practice, and we're trying to build things together.