Practical Lessons in Peer Code Review

http://blog.salsitasoft.com/practical-lessons-in-peer-code-review/

by mgertner • 12 years ago

11 4 12 years ago

blog.salsitasoft.com

tcopeland• 12 years ago

If you are including third-party code in your patch, commit it separately.

Sometimes I'll do that with changes that involve new service client libraries. First commit will have the new library fooservice_client along with its config files and such. Once that gets deployed I can open up a production console and verify that the app can query the service and display the results. Then a followup commit will contain the actual change that I want to make - "query foo service for a list of bars and display them" - and I'll put a note about how the connection credentials and setup have already been verified.

Of course that depends on an environment with more or less continuous deployment. If deploys are two weeks apart this approach is just going to take to long to get anything done.

comatose_kid• 12 years ago

My recommendations when checking in a new feature or substantial change:

1. Meet in person

2. The coder should print out copies of the diffed code for each attendee

3. Have roles in the meeting - someone to read the code, someone to capture bugs + severity, and the rest are there to review the code

4. Coder takes documented defects, fixes them and reports back to the group with diffs.

5. Metrics can be kept, used as feedback to improve process.

Usually 4-5 people are sufficient for a large chunk of functionality.

Is this process slower than gerrit? yes. Do you get better feedback, and better understanding (at a team level) of the code base? yes.

I haven't seen software that really emulates this experience - there is an interesting opportunity here.

xyzzy_plugh• 12 years ago

This is very much a cultural thing. I don't necessarily agree that you get better feedback or better understanding through an in-person meeting.

In my experience tools like gerrit allow developers to learn and review at their own pace, and provide well-articulated, thought-out feedback.

Note also that a large amount of modern development still occurs on mailing lists.

It's all about culture.

lberger• 12 years ago

"It is instructive to consider pair programming, which is a fairly popular practice and adds an overhead of 100% to all development time, as the baseline for code review effort."

I couldn't disagree with this line more. I've frequently seen it halve the amount of development time.

Practical Lessons in Peer Code Review

Related Stories