I'd like to know how often people are doing code reviews and how often you think it is right to do a review. Also, I would like to hear how you perform your code reviews. I'm using TTC for now and I'm quite satisfied with it, but I do not think it is covering all the parts of the code.
+1
A:
Before check in. Check in is often connected with logical finish of function/module (depends on the programmer), so this is a good time. When code is checked in it is already difficult to track down who coded what and when.
Svetlozar Angelov
2009-08-24 13:10:12
I completely disagree that a code review must take place before a check-in. Frequent check-ins are something to be encouraged, and a code review, although an essential tool, can only get in the way of this.
anon
2009-08-24 13:27:49
@ Neil, could you answer with your alternatives plz?
David Archer
2009-08-24 13:29:47
It's also a good idea to have each review of a size your team is comfortable with. You're looking for a balance between size of the changes and frequency at which reviews are done. Chunks too large, and reviewers lose interest and may gloss over rather than paying close attention. Too frequent reviews might be procrastinated on in favor of the reviewers' actual work.
Mike Mazur
2009-08-24 13:40:07
Actually would like to answer this myself :-), I think i got Neil`s idea. It`s not about small reviews, but about general code reviews, when whole team is getting together and starting review to understand what was done good and what was done bad and how to make sure next time they`ll have the better results. And before checkin it is enough to give your code to 1-2 team members to look through. Major code review can`t be done any time and this is right as letting people have a quick look on what are you going to check in and major code review are different things.
Yaroslav Yakovlev
2009-08-24 13:44:07
You should be checking-in in a sandbox that doesn't require review. Check-in is (very very) good, and should be encouraged without interruption. If you wait for review before checkin, you're losing a lot of the benefit of a revision control system, as software does not evolve just between "approved" revisions - there are steps in between that you don't want to lose.
Nick Bastin
2009-08-24 15:20:04
A:
Any time any code going to be checked in, so before the check in. However you might want to do it more often than that depending on the project.
If somebody is working on a project that will take weeks and they will not check in any code for that period the code should be received as various components of the project is done. The reason for this is it will have the code continually reviewed to make sure a bad design pattern is not repeated through the entire project and the changes needed from the review are less.
I would also suggest the more junior a developer is the more often the code should be reviewed.
David Basarab
2009-08-24 13:24:05
Maybe a better way is promotion to the parent branch rather check-in.
David Basarab
2009-08-24 14:06:34
+8
A:
We always perform a code review before code is committed to the trunk. I agree with Neil Butterworth's comment that frequent commits are something to be encouraged and that requiring a code review before every commit inhibits this.
The exact conditions that will work depend on the environment and the project. Our environment is such:
- Keep our mainline of development in the trunk
- Have a rule that trunk must always work (i.e. don't commit things to the trunk that don't compile or fail tests, etc.)
- Branch for each case (feature, bug fix, etc)
- Perform a code review when the work in a given branch is finished
- Merge the branch down into the trunk when it passes review
- When a release is made, we tag the gold release revision and branch. All new bug fixes for a specific release take place in that branch. Release branches are never merged back down into the trunk.
In our environment, this allows the developers to commit often (within the confines of their own branch) and for code reviews to be performed every unit of work (but not every commit).
As to How to conduct a code review, that is a much more ambitious question. One that deserves it's own question on SO (and there are actually already several):
http://stackoverflow.com/questions/89163/how-to-conduct-a-successful-code-review http://stackoverflow.com/questions/457534/best-peer-code-review-software http://stackoverflow.com/questions/tagged/code-review
Burly
2009-08-24 13:42:20
Interesting approach, what tools do you use to streamline this so that there aren't a gazillion steps involved when starting work on a bug/feature/whatever?
macke
2009-08-24 13:59:19
+1: This is pretty much standard practise for all projects I've worked on. Easy and simple.
Jeff Yates
2009-08-24 14:55:41
We use FogBugz for our case tracking and Subversion for our source control. When we begin work on a case, we just branch from trunk with TortoiseSVN and switch our working copy. Then when the work is complete and ready for review, we just assign the case to the reviewer. When the review is complete, the original developer merges it down into the trunk with TortoiseSVN. I believe FogBugz 7 (we are still on 6) provides workflow management, so you could automate a bit of the workflow, but not the SVN steps. We haven't found that it is difficult or tedious to follow this workflow.
Burly
2009-08-24 15:03:55