Code Review

Question

What code review policy you could recommend as the best for project you're managing?

Answer 1

A lot depends on your team makeup. Personally, I have had success doing pair programming and switching your pairs every couple of hours. If done right, this can be more efficient than a code review.

Another method is just getting a few team members together with a projector and going through the code every Friday or some other time frame.

Answer 2

In general, try to have them rather than not having them.... every time I have a code review, I almost always get some really good advice to tweak something in a good way, or remove some duplication I didn't see, or even some optimization in how I use my environment (shell, IDE, Emacs, etc).

A coworker once suggested we all get together in a room once a week and review major changes we did, briefly. I imagine that will help too, but I can't imagine it replacing 1 on 1 code review for major checkins (even brief ones will prove beneficial).

Answer 3

Here's the code review policy we use at work: http://www.divmod.org/trac/wiki/UltimateQualityDevelopmentSystem

Short version: changes are developed in branches, which are reviewed (and checked for tests passing) before they're merged. If the merged code causes a regression, it's reverted from trunk.

Answer 4

review only the code changes from the base code and use some free file comparing tools.

Answer 5

We have 5 developers at differing levels at my organization; as such, code review is sometimes more of a teaching tool than a review, per se'. However, by sharing code between developers (and by diligent use of source control) the quality of code has gone up significantly.

I would say that a good way to get developers working together and learning from one another is to make it a sharing experience, not so much a review. It puts a different spin on it and I believe developers become a little more willing and less sensitive about their particular snippet(s) of code. YMMV.

Answer 6

I would agree with SaaS Developer that pair programming yields good results. However the question depends on other factors as well such as team size and development model for example. Lastly, the following link will direct you to a code review web tool you might find useful.

Rietveld

Answer 7

My team is too small and the code is too much to review via a formal review process.

Instead what I have found effective is using the versioning system to email diffs when emails are sent. SVN/CVS->email goes to all of the team members who partially out of curiosity will review submissions. On top of this, code presentations by developers for major modules is effective at showcasing some of their work.

Each check-in is tagged with the bug ID or feature request (for new code), and that allows some traceback.

Lastly QA's involvement means their testing of the code will provide some validation.

As for the efficiency of the code, that's done a bit more statistically as we don't have the time to review algorithms. Eventually I'd like to tie in our load testers to the process and have them run the automatic load tests more frequently.

Answer 8

"as the best" depends on so many factors:

1. The product itself. Is it life/mission critical?
2. The cost of fixing post-release. Compare the cost of a PC software patch on a dozen machines vs. recalling millions of smartcards with an embedded code defect.

These types of factors should drive what the appropriate level of code review is. Depending on need, it could be informal checks, checklist-based review, pair programming, or intensive Fagan-like inspections.

Answer 9

We use pair programming and we tend not to do code review with the whole team present, but architecture reviews. I also try to sit with a programmer and do some general code review of important features or when they are using a new tool/technology. We also document best practices and patterns distilled from this reviews in an internal wiki.

Answer 10

Our code review process is very straightforward. Before a fix/feature is released, it is highly recommended that it is reviewed. Reviews are done by "the person to your left" and they just open subversion log window and check out the diffs. They make their comments to the original author, and prepend a "+" to the log message to indicate that it has been reviewed and then put their initials at the bottom. This makes it easy during the release process to identify specific commits that haven't been reviewed and we can divide out the unreviewed commits before the code is released.

Rather than throwing people in a room, we figure it's better that at least someone has looked at it, since two pairs of eyes are better than one.

Answer 11

No code should be checked in without at least one review. It will slow things down at first till you realize that you have more balls in the air, but you save yourself from doing something stupid and someone else is now somewhat familiar with your code.

/Allan

Answer 12

We mandate the use of tools wherever possible, mainly to try to reduce the overhead of code-reviews. With tools to lighten the boring syntax checking, the reviewer can concentrate on design issues.

Cruicble by Atlassian is fantastic for doing code reviews "online".

Findbugs / PMD / Checkstyle remove a lot of the fiddly syntax checks a developer has to do.

Eclipse / IntelliJ IDEa contain incredible inline warnings.

Whilst I would love to go completely agile, we can't due to a myriad of reasons I am not going to go into here. However I will point you at a paper giving some insight into some of the "agile" techniques we have tried to incorporate into a "waterfall" project with what I think is fairly reasonable success.

http://www.aswec2008.curtin.edu.au/IndustryReport/Morgan%20-%20Agile.pdf

Answer 13

We do distributed development, so sitting together in a room is impossible when we're hundreds of miles apart. So we have set times twice a week when we go through code, everyone connected to a shared desktop machine.

Anybody can bring code to review, no one is exempt from reviewing, all code should be reviewed before being put into the main thread. And everything is open for review, from testware to test cases to source code. If it's worth doing, it's worth talking with the team about and worth having other eyes looking at it.

We walk through and discuss everything from simple bugs to source file organization to coding standards (we're early enough in the project that these are still evolving). Not a formal inspection (though we do those when the team decides it's necessary), so it's author-led. We do comparisons to previous versions and sometimes bounce from source file to source file in order to answer questions.

Meanwhile, we do pair design and development on an ad hoc basis. Pairs evolve from day to day and week to week as we progress through the project. Again, because folks are remote, it's done through shared desktop connections and the like.

Answer 14

The psp one

Answer 15

I worked on a project that required another set of eyes on any code prior to committing. The commit log had to indicate who the code was reviewed by.

Answer 16

We have integrated the code review process as a workflow step in Jira. When a developer commits changes to the mainline they mark the issue as fixed in jira and it is sent to someone to code review. They can reject or send it on for testing.

There is always almost something found that could be done better when you code review. It's much better to code review before testing when you are more suspicious of the code so that you do not start with a mindset of it working.

Answer 17

I use the Trunk as "Done" branch (i.e. releasable) and include Code Review in the "Definition of Done". In other words, code can't be merged in the "Done" branch as long as it hasn't be reviewed.

Answer 18

My favorite is adding [Reviewer: name] to every commit comment. This way, you make sure there's more than one person in the company who knows what was done.

If there's a problem with the commit, you ask the reviewer first. This makes sure that reviewers fully understand the committed code.

Answer 19

rietveld seems interesting, based on Mondrian, a web tool for code review developed at google by Guido Van Rossum (of Python fame)

It allows code reviews to happen at any time and does not require face to face contact.

http://code.google.com/p/rietveld/wiki/CodeReviewBackground

demo here
http://codereview.appspot.com/