fwd: Changing the review culture

[Martin Pool]

I thought this was a pretty interesting insight:

On Wed, Sep 10, 2008 at 10:27 AM, Christian Robottom Reis
<kiko at async.com.br> wrote:
> I guess you shouldn't have offered to massage as all you get is joycian
> blabbering.. Francis, this is me and Barry chatting about how to make
> reviews work better in LP, for discussion in tomorrow's meeting.
>
> - Core insight:
>    - Reviews are a mediocre QA tool
>    - Reviews are a fantastic learning tool
>    - Except when you don't actually use them to learn or spur learning
> - Most of the frustration people feel in reviews is either that
>    - they take too long
>    - they are too hard
>    - they change code you don't know
> - As a result, people spend a long time reviewing, check out code, run
>  tests, etc
> - As a result, they get frustrated at the overhead -- and that's both
>  psychological and cognitive overhead
> - As a result, they learn less, want to review less, and actually end up
>  not causing the thought process that leads to better code
> - How do we change this culture?
> - Easy. Stop doing this overhead stuff and focus on learning
> - Just ask questions
> - And then ask more questions
> - By asking questions:
>    - The person who wrote the code goes back to the code and says hmmm,
>      is this actually doing the right thing?
>    - In the worst case, the person gives you a great explanation, you
>      have learned and both are satisfied
>    - The more questions you ask, the bigger the risk of you getting the
>      Inquisitors Award at the Launchpad Epic!
> - Your review shouldn't last more than 30 minutes for a large branch,
>  and no longer than 15 minutes for a short branch.
> - To do this, improve your scanning ability
>    - Scan through the patch
>    - Identify changed code
>    - Ask a question
>    - Move on
> - Concentrate on the stuff which looks weird. If it looks weird, just
>  say "this looks weird. did you think of alternative ways of doing
>  it?". You don't even need to know what you are talking about!
> - Don't worry so much about what comes after the code being changed. If
>  you see code changing a behaviour and are unsure of test coverage, ask
>  immediately "Is this tested properly?". As you scan through the rest
>  of the patch, if you see a test for it, you can come back and delete
>  the question, or just say "Ah, I see the test for that code I asked
>  about. I wonder if it is testing exactly the change you made -- what
>  do you think?"
> - If you are unsure about something, don't research. Ask. The diff
>  should tell you everything, and if it doesn't, the first reply you get
>  should. If it doesn't, keep on asking. The coder has the burden of
>  clarifying -- not you.
>
> And finally:
>
> - If you have time or are bored and want a challenge, try rewriting one
>  of the weird sections. But don't invest too much time, because the
>  coder has much more context than you do, so you are at great risk of
>  getting it wrong.
> - There are special cases of course. You might want to check out a UI
>  change (though you can bother the coder to put up a test site). The
>  coder might actually be asking you for your opinion on something,
>  which might require some research. But those are a minority of
>  branches.

-- 
Martin <http://launchpad.net/~mbp/>