TFS Shelvesets and Code review [closed]

Question

One of the reasons for using TFS shelvesets is for code review which I don't agree, but this is the practice followed in my current project. The reason I see using TFS shelvesets is not a good idea for code review are

• Shelvesets do not have a natural ordering as change-sets have. This can lead to lots of merge conflicts.
• If developer cannot checkin code until it gets reviewed, it puts a dependency on the reviewer, and if the reviewer does not do review within a short duration these shelvesets can interfere with other tasks.
• Collaboration with other devs becomes pain as now you need to pass shelvesets around rather than checkin code which can again cause merge conflicts in future.

Can someone provide me some pointers which can be either for or against the TFS shelveset approach for reviews so either I get convinced about the approach or I can present a case for not using this approach?

Answer 1

I think Microsoft disagrees with you a lot on this one, as the new Code Review feature in TFS 11 and Visual Studio 11 is built around shelvesets. The real issue is probably more around how the team operates and how tasks are split up across the product.

If the tasks have been split such that they have few dependencies and such that the people working in the same area work closely together, then you won't have any issues around merges and checkins. Should you have tasks taking more time, then regularly take the latest version from the development branch so that you're always current.

If you see that the Reviewer is too slow and that shelvesets are queueing up, all waiting to be reviewed, then this is where the other real problems can occur. When a task is finished it should be reviewed as soon as possible and not lie around waiting to be reviewed. If reviews take longer than 24h, then this can become a real issue. You could alleviate this by doing a peer review by someone else or by getting more reviewers on the team.

If all else fails, you can do post-mortem reviews (review the changesets instead of the shelves), TFS 11 and Visual Studio 11 support this scenario as well.

My personal preference is to trust the devs on my teams and we thus mostly do post-checkin reviews. If we have new or very junior members, then I'll make sure that a more senior developer is available to do a first pre-checkin review instead.

See also:

• https://jessehouwing.net/community-techdaysnl-did-you-miss-my-talk-on-codereviews/
• https://blogs.msdn.com/b/ivo_manolov/archive/2010/11/29/10097641.aspx?Redirected=true

Answer 2

Shelveset do not have a natural ordering as change-sets have. This can lead to lots of merge conflicts.

I can't see your point here, what is a "natural ordering" for you ? The chronology of changesets don't follow a given order when you start to work in a team.

If developer cannot checkin code till it gets reviewed, puts a dependency on the reviewer and if the reviewer does not do review within a short duration these shelvesets can interfere with other tasks.

Again, you have the same situation with "regular task development" that's not because you start a task A before a task B that you will check-in the task A before B (unless B is dependent from A, but that's not the point here). Consider review as the final step of the workflow of task developement. The dependency on the reviewer indeed make things a little more complex, but it's for the sake of having a stable build and have code compliant to the company's standards.

Collaboration with other devs becomes pain as now you need to pass shelveset around rather than checkin code, which can again cause merge conflicts in future.

Do you know something easier than shelvesets ? Do you prefer sending an email with the modified code in a zip file ? Shelvesets are by far the easier way to share code between developers when you don't want to impact the referential. Here again, I can't see the merge conflicts issue you mention.

Here are some advices:

1. When someone is getting back the shelveset of another dev, say Dev A created a shelveset and Dev B want's to review it, make sure Dev B has a separate clean and dedicated workspace to unshelve. You don't want to unshelve things in your regular "dev" workspace. A dedicated workspace for code review ease the merge conflict issue you mentioned.

2. In theory, everything should be reviewed before being integrated to the target branch. That being said, it's harder in reality to do so, so don't aim to do something perfect if your team doesn't have the habit of such processes. A Senior Dev that knows well the application he's working on can be authorized to check-in before review. It's all a matter of tradeoff, in this case you gain flexibility and a smoother dev experience but your referential can suffer in quality and stability. There's no real winner here, it's your choice based on what's important for you.

3. Don't use branches for code review.

4. I agree the code reviewing experience through shelveset is somewhat incomplete in VS/TFS, but it's way better to the alternatives. Microsoft realized that they could do better in this regard and it translates in the improvements made in VS11/TFS11. With the next version you have a true code review experience, still based on shelveset but with a more complete communication system between the actors. This improvement was made in the "My Work" experience and things are way smoother now. Give a try to tfspreview.com and VS11 beta or read some blog posts (Brian Harry) to get more info. Here's a link you'll be interested in.