Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

We need to do pre-check-in code reviews. We are using Visual Studio and TFS.

The built in code review functionality, is a bit odd to me. If I "reject" a check-in and comments what needs work, the review goes away.

When the developer then makes the changes, he requests a new review, and I start all over again.

Now, let's just say there are 10 files in this check-in. Maybe 7 of them were originally fine. Now, I have to review them again, as we are doing a brand new review. OR, I can track myself, what I have done.

I can also, ONLY compare code with the original version, not the one from the reject review.

So, my questions are:

  1. what do you guys do?
  2. What are your experiences with pre-checkin code reviews?
share|improve this question

marked as duplicate by gbjbaanb, MichaelT, gnat, kevin cline, ChrisF Jan 19 '14 at 22:07

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

What kind of branching scheme are you using? I have an idea, but it requires you to have private development branches for every individual. –  Thomas Owens Jan 17 '14 at 13:21
@ThomasOwens we are branching into the release versions, when we are ready to release. Branching is not something these developers will have access to. It is the up to the application responsible person instead, also the guy who (in most cases) does most of the reviews. Unfortunately, I am afraid private dev branches will end up with a lot of extra work. –  Nicolai Jan 17 '14 at 13:24
Does your version control system allow for merging without checking in, then? ClearCase is a good example of this, where I have a view of an integration branch and until I check in, the files are only part of my view. Although in your current branching configuration, the idea I'm left with may be pretty terrible. –  Thomas Owens Jan 17 '14 at 13:25
@ThomasOwens I am unsure if we can merge without checking in. I don't really see why I would be able to do that. –  Nicolai Jan 17 '14 at 13:29
@gbjbaanb unfortunately, that does not have any answers that relate to my questions. –  Nicolai Jan 17 '14 at 13:38

1 Answer 1

up vote 2 down vote accepted

I prefer to do the checkin, assuming the developer has done the work correctly (ie be optimistic about their code quality)

If it turns out a review requires rework, then the dev can checkin fixes - in many ways this is exactly the approach taken with bugs, commit; build; test; commit fixes. Its not a problem with releases, why should a code review be any different?

I always create a ticket for the review, just like any other task - and track the progress of it, so the dev commits, he creates a new code review task for the commit and assigns it to the reviewer who then uses it to record any review failures that need work - and passes it back to the dev who fixes and re-assigns, or closes it.

The developer should be working on a branch rather than the mainline trunk, you can then perform the review without holding him up - he can continue to work on other tasks, while he waits for you to perform the review.

In the old days I used ReviewBoard for reviews - this requires you to upload a diff (the SCM was automated to provide these and create the review tasks). Now I'd use Redmine which can integrate the review process with the SCM tool and its inbuilt ticket tracker. (you will need the CodeReview plugin though).

In all cases, review after commit - its the only way to keep productivity up.

quick edit: the benefit of post-commit include: a) if you mark the revision you reviewed, the coder cannot cheat and tweak their code when the commit it after the review... b) the SCM tool gives you a diff of all the changes made since the last review, making review easier. c) waiting for the review doesn't block the dev from working.

I would work with branches in this case anyway: dev works on a branch, it gets reviewed and merged onto a 'for testing' branch, is tested and then gets merged onto a 'for release' branch. This way, you can easily see that the process for review is exactly like the process for test so you can manage your entire delivery process the same way.

share|improve this answer
As much as I dislike the idea of post-commit reviews, without private development branches, I think that going down this route may be the best option to maintain productivity and not block other people from committing until reviews are complete. –  Thomas Owens Jan 17 '14 at 13:32
Even with private (or feature branches) its ok to review post-commit. After all, the worst that can happen is that you have multiple commits for a particular task (which is how many people work anyway). Ok, you might be tempted to ignore the reviews.. but that's an entirely different kind of problem. (oh, and a huge benefit of a post-commit review are the diffs that the SCM gives you - without any cheating being possible) –  gbjbaanb Jan 17 '14 at 13:39
@Nicolai the reason is that only reviewed code gets merged onto a more senior branch. So the dev can make as many mistakes as he likes and it won't affect your version history - in the release branch. It also means he can code many different tasks on his branch whilst waiting for reviews - when each passes review, only those revisions get merged across. Working in main just means you have a lot more work to do managing which revisions go with which task, and they will get mixed up together. –  gbjbaanb Jan 17 '14 at 13:47
In addition to what @gbjbaanb said, if you have branches for closely related bugs/features instead of a single development branch per individual, then you don't need to worry so much about selective merging. After a review, you can (usually) dump the development branch into an upstream branch. –  Thomas Owens Jan 17 '14 at 13:50
@Nicolai my pleasure. now, we just have to get you off TFS and onto something really good :-) –  gbjbaanb Jan 23 '14 at 10:48

Not the answer you're looking for? Browse other questions tagged or ask your own question.