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.