The Joy of Code Reviews

re: The Joy of Code Reviews

daniel — Tue, 06 Jan 2009 15:25:48 GMT

I am a big fan of"code review". I have been pushing this along within my team as well. However, without trying it myself, does your team feel that the approach of yelling out "code review" may be quite an interrupt to the whole team. For instance, this could break everyone's zone.

My team uses VSTS shelveset. Reviewee emails the set name to a reviewer. Reviewer does it for a break. Reviewer grabs the reviewee if there is any clarification needed. But my team approach has one big issue is that the reviewee may be waiting for quite a while until the review is done, which is quite annoying sometime.

What do you think?

re: The Joy of Code Reviews

Hal — Tue, 06 Jan 2009 20:01:40 GMT

What about continuous integration? I would think that the efficiency of the process you describe could be improved if devs first checked into a CI branch where automated unit and functional tests were run. On success devs could then peer review merges to the main line of development and trigger a new round of automated testing.

re: The Joy of Code Reviews

Bob — Tue, 06 Jan 2009 20:30:31 GMT

Hal you make an excellent point. What he is describing could definitely be automated at least partially through a continuous integration server.

re: The Joy of Code Reviews

tomholl — Wed, 07 Jan 2009 00:22:21 GMT

Daniel - noise is not uncommon in our team room, and the odd shout is probably a lot less distracting than other things that happen (my singing, for example). Having someone review a shelf-set on their own computer would have some value, but it doesn't address the goal of our code reviews. The developer-to-developer interaction is the most significant part of the way we do this.

Hal/Bob - as per my earlier post on how we do agile, we do run a CI build, but this happens after the reviews are complete. The CI build is important but picks up a different class of issues to code reviews. Developers will only ask for a code review if they have high confidence that the CI build will succeed (i.e. all the code builds and tests pass on their own machine).

VSTS Links - 01/07/2009

Team System News — Wed, 07 Jan 2009 17:04:40 GMT

James Whittaker on New Year's Resolutions Tom Hollander on The Joy of Code Reviews Ayman Badawi on...

re: The Joy of Code Reviews

Alby — Thu, 08 Jan 2009 02:29:15 GMT

Code review is awesome (done in the right spirit anyway). Have you considered an online code review tool like Reviewboard, Codestriker, Atlassian Crucible, Smartbear Codecollaborator.

re: The Joy of Code Reviews

Carel Lotz — Thu, 08 Jan 2009 14:29:44 GMT

Alby

I agree, the right spirit is key to the success of code reviews. Being collocated is IMHO another key ingredient for optimizing the cost/benefit factor of doing the review. I am currently working in a distributed team and this makes the review process a lot more painful and cumbersome.

re: The Joy of Code Reviews

Jon Kruger — Sat, 17 Jan 2009 06:29:46 GMT

Did you consider having everyone pair program all the time (sort of an ongoing code review)?

Have you noticed a difference in quality (fewer bugs) since you started doing the code reviews? Have you noticed if it's taking longer to get things done?

Just curious because I've been thinking about this kind of thing a lot lately and I think I'm ready to take the plunge.

re: The Joy of Code Reviews

tomholl — Sat, 17 Jan 2009 07:30:15 GMT

Jon - The code review process I've described here was entrenched in the team's culture before I joined, so I can't offer a "before and after" comparison within the same team. I think the major benefit of the approach is to improve code quality and maintainabiliy more than to reduce bugs, but there have definitely been cases where potential bugs have been picked up by code reviewers.

We do use pair programming at times (primarily for complex tasks), but in my experience it isn't an effective use of development resources for more routine requirements.

re: The Joy of Code Reviews

cellfish — Sun, 25 Jan 2009 00:53:28 GMT

I don't think code reviews can be automated using continous integration since the purpose of the code review is to have another set of eyes take a look at the code and see if the design can improve and/or spread knowledge of the code. The only thing that does this "better" is pair programming IMHO.

And regarding the shout outs vs. having people review code using tools/shelveset by them selfs it takes an extremly competent reviewer to actually ask those questions at a later time. But if the reviewer is sitting with the author the reviewer more easily ask questions. In my experience face-2-face reviews result in more comments and discussion than if the reviewer sits alone. And more comments and more discussion are generally better I think.