Peer Code Review with Team Foundation Server 11 – Part 5
Action: Requester Closes the Review
Back in the requester’s Code Review pane, I can see the status of each reviewer’s responses and their finished response (e.g. Looks Good).
At a glance the requestor can see any overall comments made. (I like that you can see how long ago a comment was made as well.)
If a requester wants to inspect inline comments, they can click the file name to open the diff tool. Commented code is highlighted in yellow.
The requester can continue to make changes and submit new code reviews. As long as they haven’t committed any code, the reviewers will still be comparing all proposed changes to what’s stored on the server. (See example screenshot below.)
Under the Close Review hyperlink, the reviewer has the option to click Close or Abandon to finalize the code review.
Closing a code review sets the State and Reason on the Code Review Request work item to Closed. Any Code Review Response work items remain unchanged.
Abandoning a code review also sets the State and Reason to Closed, but the Closed Status field is set to Abandoned. Linked Code Review Response work items also have their State and Reason set to Closed, but the Closed
Status is set to Declined and a Closing
Comment indicates that the requester closed the code review.
I think this is a great feature. There is a lot of value in having code review integrated within the developer’s IDE. Code reviews should be easy and encouraged. Frequent code reviews not only catch bugs and improve maintainability, but they reduce the all-too-common ego that comes with the craft of writing software. Today’s serious application development is a team effort and the ownership of any new feature or bug fix should not be on one individual.
Here are some other features I’d like to see considered for future versions. Some of these could probably be done by editing the work item types and/or utilizing the TFS API.
- Mouse-over comments – I think it would be a better experience to be able to mouse over the code in the diff tool and see the comments.
- Configurable enforcement – Some organizations want every change to an application reviewed prior to release. It would be nice to have some setting to require code review. E.g. One setting might enforce a closed code review prior to check-in. (I’m going to try to develop this type of enforcement for Part 6 in the future.)
- A Reviewers group – Some organizations designate one or more individuals as mandatory reviewers. It might be nice to have a list of these reviewers that could somehow designate a change as reviewed. The add recent reviewers capability is similar, but an organization may want more control over this group.
- Review merged changesets – I’ve always tried to encourage my development teams to check-in code very frequently. This keeps changes atomic, but I may not want to overburden reviewers with incomplete thoughts so it would be helpful to select multiple changesets (or by label).
- Lync – It seems like Lync could be incorporated to provide richer conversation options. At its simplest, I could tell if a reviewer or requester were online. It’d be interesting to somehow attach those conversations to the code review as well.
- Additional reporting – It’s probably fairly straightforward to build with Excel hitting the data warehouse, but I think it would be more usable to right-click a file and not only see its version history, but all of the associated code reviews as well.
What do you think? Did the product team hit the mark with this? Do you value code reviews by your peers? How do you execute code reviews today?