Second code review is somewhat painful

Jan 15, 2010 at 4:53 PM

We've been using the latest version of Team Review for about two month now and are really starting to get into using it and it is great for a 1st review and update.  My problem is that when coming back to a 2nd review of the code, it is not that easy to see exactly what was changed.  Here is our process:

  1. Coder shelves a changeset and assigns task to reviewer
  2. Reviewer unshelves on to local box and reviews code (creating code review responses (CRR) as necessary)
  3. When done reviewing, Reviewer assigns task back to coder
  4. Coder goes through Replay Code Review and changes code based on CRR
  5. Coder shelves changes and assigns task back to reviewer
  6. Reviewer unshelves on to local box and reviews changes based on CRR (creating CCRs as necessary)
  7. Repeat steps 3-6 until there are no CCRs open

The problem is in step 6, the CCR may no longer point to a proper location in the code since the line numbers in the code have changed.  Therefore, it is somewhat difficult to find the exact change that the coder made in the second round.  It is almost as if you have to re-review all of the code again in this scenario.  Has any figured out a better way to review the changes that were based on the CCRs.

PS: I recognize that utilizing shelves may be causing our problems and that if each coder worked in their own branch, then the coder could check in their code and the reviewer would be able to see each and every change a bit easier.  That would be a major change to our processes, at the moment, so I'm kind of resisting doing that.

 

Jan 16, 2010 at 1:26 AM

Hi Kipp - I do think that the mechanics of shelving twice is contributing to the problem, because there is no difference-view between the first shelve and the second. However, that's the way it is, and with TeamReview being an after-market add-on it would be lame of the TeamReview contributors to claim that as the root problem. 

This may be long, but if you can wade through it I would greatly appreciate the feedback. Tell me if these theoretical upgrades to TeamReview would work for you:

  • The Code Review Repay work item is renamed to "Code Item"
  • A new "Code Item Type" is added to "Code Item" with the default values of "Review Request", "Review Response", "Technical Debt", and "Security Issue" (ideas welcome on other defaults)
  • When you right click in the code and get the TeamReview menu the new options are "New Code Item" (old new code review response) and "View Code Items" (old replay review)
  • When you chose "New Code Item" you now select all the current fields but also a value for "Code Item Type

Your workflow above then could look like:

  1. Coder shelves a changeset and assigns a new "Review Request" Code Item for each changed area by highlighting the code he/she changed and associating it with the original Task/Scenario/Bug
  2. Reviewer unshelves on to local box and uses the "View Code Items" (old replay code review) to see "Code Items" created in previous step and subsequently creats Code Items with type of "Review Response" as necessary (no more searching for what to review, TeamReview takes you there)
  3. When done reviewing, Reviewer assigns the Review Requests (that the coder entered) back to coder
  4. Coder goes through Review Responses (entered by Reviewer) and changes code based on those Code Items
  5. Repeat 1-4 until there are no open Review Request or Review Response Code Items open

It's an idea that's been recommended to us, and that we've been tossing around. The challenges are the renaming of the work item type, and compatibility issues of new clients to old work item types and vice versa.

 

What do you think?