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.

bzr-removable

The plugin I talked about in a previous post is now at https://launchpad.net/bzr-removable. Please file bugs.

Thanks to Michael Hudson and Daniel Watkins for submitting patches.

You can get the plugin by typing 'bzr branch lp:bzr-removable' on the command line.