Proven practices for more effective, efficient peer code review | Hacker News Reader

Proven practices for more effective, efficient peer code review

ibm.com

80 pointscpeterso18 comments

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

> 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?

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/ [2] http://www.reviewboard.org/

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.

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.

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

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.

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.

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)

https://en.wikipedia.org/wiki/Appeal_to_motive (sorry, I just think this article deserves a more kind top comment than this one)

Anything that's from "Rational Systems" is an overpriced unusable piece of crap and I wouldn't touch it with a ten foot pole

Not disagreeing, but the tool mentioned is not from Rational, it only integrates with the Rational workflow tool.

I've used CodeCollaborator (the SmartBear software plugged at the end of the post) and found it to be a good tool for doing multi-site code reviews. It's straightforward to use; web based; the default diff tool is reasonable; you can specify suggestions or bugs i.e. "I think you should change this line" versus "This line needs to be changed".

IBM stuff, however... well, I've mentioned my feelings on their software on HN before. They're still selling Lotus Notes and Clearcase - I'd be very careful before taking anything they've built/bought.

There are a bunch of open source software to do what exactly what they advocate, that is "a tool-assisted lightweight code review".

At Mozilla, we use GitHub (for project hosted on github, such as Gaia), and splinter [1], that integrates with bugzilla. I know there is an ongoing effort to switch to reviewboard [2] at the moment, as we feel our review tool should do more that what splinter is capable of doing.

[1]: http://blog.fishsoup.net/2009/09/23/splinter-patch-review/ [2]: www.reviewboard.org

//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.

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.

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

Yeah that isn't a good thing, because by the time your code is reviewed it would no longer compiler as others have made changes in the code too.

Reply on news.ycombinator.com