What I meant

z3p recently blogged about a comment I made in a code review. In the review, I linked to:

That’s a negative and grumpy way of phrasing an idea that I’ve come to value a lot: good code expresses its intent clearly.

When looking at a patch, the reviewer needs to understand two things: the intent of the code and the intent of each change to the code. To be clear on the former, you need:

• intent-revealing names.
• good abstractions / interfaces.
• good, small tests.
• simple implementations where possible.[1]
• docstrings where appropriate
• comments where appropriate.

That’s not exhaustive, but it’s in a rough order.

To be clear on the intent of your change to code, you need:

• Small patches.
• A good bug / spec with a good, short summary.
• A review request letter, summarizing your implementation strategy, any compromises you made, gaps in testing, future work etc.

That’s not exhaustive either. In #2037, I didn’t understand the motivation for lots of the code, nor for some of the changes to the code.

I’m indebted to Andrew Bennetts for teaching me that the first duty of a reviewer is to ensure that the code is clear and to Tom Berger for reminding me that compromises are worth noting.

[1] Actually, this reminds me of something I heard a preacher say, “before I give a sermon, I go through it, find everything clever, and take it out” (I paraphrase, not having a reference on hand).

In as much as sermons and code should both be ego-free communications of ideas, I think this is sound advice for hackers.