

Proven practices for more effective, efficient peer code review - cpeterso
https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/

======
thomasahle
It's great to read well documented studies like this. Most blog posts on such
topics tend to be based on very little evidence

------
mfonda
> Foster a good code review culture in which finding defects is viewed
> positively

Any tips on fostering this sort of culture? I personally am vary grateful when
others point out defects in my code, as I view it as an opportunity to learn
and grow as a developer. I try to encourage other developers on my team to
have this attitude as well, and most do. However, I've worked with developers
in the past who view anything pointed out in code review as personal attacks
rather than a chance to learn. What can I do to help avoid this?

~~~
Aga
I've worked in a couple of big projects (250+ developers) where we have
introduced Gerrit Code Review [1] that has mandatory code reviews.

In all cases, there has been a lot of initial scepticism and worries.

Some things that I feel have contributed to people accepting code review, are:
1) Gerrit enforces code review, you have to get it done somehow, in order to
get your changes in. It's integrated in the workflow. 2) You choose your own
reviewers. 3) We have had a clear message, that code review is about
communication, knowledge sharing and bug hunting is just an added bonus. And
communication is always our biggest problem in big projects. Just by letting
someone do code review of your changes, the knowledge inside the organization
about the change has almost doubled. 4) We ask people to treat code review
request as number one priority, as the changes can not get in before it.

Before Gerrit we tried to encourage the culture by using Reviewboard [2], but
in our case it just wasn't used that much. People did not treat review request
as a high priority. Maybe because it was not enforced and sometimes you'd do a
review for a change that the developer already merged in.

It's interesting to see how small tricks can change peoples behaviour...

[1] [http://code.google.com/p/gerrit/](http://code.google.com/p/gerrit/) [2]
[http://www.reviewboard.org/](http://www.reviewboard.org/)

------
vambo
The author preparation feature (#4) could indeed be useful, sometimes it takes
quite a bit of time, especially if one is not very familiar with the system,
to see how they fit in.

~~~
tetha
We have implemented this using something we call the overview meeting, or the
architecture meeting.

It is an optionally requested meeting at the beginning of the review process.
In this meeting, the goal, the general structure of the software and the
general structure of the changes are discussed on a high level for about 10
minutes. Afterwards, we decide which classes are focus in the review, which
classes are kinda part of the review but boring. Sometimes the review is even
removed because the changes are too small.

Overall, it's a very helpful meeting, especially for people new on a project.
Confusion changes into understanding and focus with just a few words and a
simple informal diagram and from there, very precise and productive code
reviews follow.

~~~
vambo
Yes, this sounds like it is well worth the time. Not that in my current
project it would be unthinkable to go and ask the original coder for some
extra info but people tend to be busy and it tends to feel a bit like wasting
their time.

* minor correction to my original post - "they" refers to the code changes

------
GregorStocks
The final tip being "use the tool I'm selling" makes the preceding 10 seem a
lot less compelling. I'll stick to testing my code in prod, thanks.

~~~
nekgrim
The 10 first tips can still be applied without the 11th. And a mail thread can
replace their tool. Less efficient, but free. And still more efficient than
testing in prod.

~~~
masklinn
And the 11th tip is to use a code review tool, not to specifically use the
example one (although the plug/mention is a bit heavy-handed)

------
pramalin
//All code had to be reviewed before it was checked into the team's version
control software.// Their first rule above is based on how incompetent their
RTC tool sets are. I wouldn't say this is a best practice.

~~~
pramalin
I would elaborate that, a version control system must assist in code review
process by allowing to version control the unverified/reviewed code also in
private branches. With RTC, it is hard enough to review the change history in
trunk and its capabilities pale in comparison with say Tortoise SVN.

That's why I feel that their first rule of their study is based on the
shortcomings in RTC's jazz version control system.

However the code review best practices came out of the study are solid.
However they do not touch upon the version control practices.

~~~
jacobparker
Sorry for down-voting your original post, I misunderstood. Your clarification
makes sense and I agree.

