4

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?
Improve this question
8
  • 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, 2014 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, 2014 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, 2014 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, 2014 at 13:29
  • @gbjbaanb unfortunately, that does not have any answers that relate to my questions.
    – Nicolai
    Jan 17, 2014 at 13:38

1 Answer 1

Reset to default
2

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.

Improve this answer
8
  • 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, 2014 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, 2014 at 13:39
  • 2
    @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, 2014 at 13:47
  • 1
    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, 2014 at 13:50
  • 1
    @Nicolai my pleasure. now, we just have to get you off TFS and onto something really good :-)
    – gbjbaanb
    Jan 23, 2014 at 10:48

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