views:

111

answers:

7

Disclaimer I don't do code review, so this question is purely of academic interest.

I have seen plenty of posts on stackoverflow that advocates/requires code review before it is allowed into source control.

If you do this, where do you store code the unreviewed code, and how do you deal with the issue where the developer has to update the code to deal with conflicts resulting from other checking - do you require him to get his code reviewed again?

Thank you.

+3  A: 

Code that isn't reviewed is still tested. And trust is an important thing in development. so the answer is that the code belongs just to the versioning system. From there some gets it for review. If there is a problem it can be rollbacked. Everything else is just a big hussle for nothing most of the time. Remember that code in the repository is not "code in production"

Everyone should update from the repository before commiting. If you detect a lot of updates it is really feasible to run the tests again. If there is no problem than commit.

Norbert Hartl
A: 

We store it on the developer's desktop. It's not checked in to SVN because it's not done yet.

If it's a lot of code, that's a problem -- you're waiting too long to review.

If it's a reasonable amount of code, it can be emailed to reviewers. Maybe it needs a ZIP file to keep it organized.

Sometimes we'll post it to SharePoint, but that's rare. Email usually works fine.

S.Lott
The code has value (e.g. for the company or developer team) even though it's not done yet. Until it's in version control, it gains none of the advantages that version control provides.
Craig McQueen
@Craig McQueen: our position is that it doesn't have value until it's reviewed. The first review almost always leads to rework. In a few cases significant "discard that module and do it this way instead" kind of rework. Whether the code actually has value or not is just a policy decision, and we made a different choice.
S.Lott
A: 

My current team does code reviews even before delivering results to stakeholders, so I'm in the camp that advocates committing to source control only after a code review.

That said, one possibility is to store a patch file in a directory on disk instead of committing to source control. Another option is to use a separate branch that changes are committed to prior to being merged to their target branch, but I fear that this approach is wrought with peril.

Paul Morie
+1  A: 

Academic answer for an academic question, since we don't do code review either.

EVERYTHING gets checked into source control. If it isn't fully working/tested/reviewed, it goes in that developer's personal branch.

Adam Jaskiewicz
+1  A: 

This sort of practice is a great reason to use a DVCS like Git. Developers can work for longer periods without committing, allowing for code reviews but still making use of version control techniques that we have come to appreciate. If you are using something like SVN you'd have to branch for every bug/feature/whatever that needs to be written and reintegrate it after code review... which could be painful.

John Cromartie
+3  A: 

Code should get checked into your repository (SVN, TFS, etc). You can setup a development (or even per-developer) branch if you want to prevent it from going into the trunk until after it's reviewed.

Jason
+1  A: 

This really depends on what kinds of tools and procedures the team has set up.

For informal code review, you can simply check it into version control and let other developers review it on their own schedules; problems found by the review are then checked in separately. (That's what our team does.)

For more formal tool-assisted code review, tools like Google's Rietveld and (I'm pretty sure) Smart Bear's Code Collaborator let you upload code for review and have a miniature version-control-style history on each submission as it's updated over the course of a review. (If you're interested in learning more about code review, Smart Bear has a free book on the subject.)

Josh Kelley