LII:Medical Device Software Development with Continuous Integration/Practical Application of Agile
Ticketing system as a trigger for code peer review
Somewhat recently I was thinking about what ticket status might be appropriate when using issue tracking for all tasks from functional requirements to documentation to defect tracking. It got me thinking about the need for peer reviews of code and how tedious these reviews can be. It turns out there is at least one plugin for Trac that includes hooks for annotation of code for the sake of peer review. It does not, however, appear to include any kind of formal sign-off capability.
I started thinking that it would be nice to have a plugin for peer reviews (for Trac or Redmine or whatever). Used wisely, however, defining our workflow in a manner that makes the peer review process an integral part of it, we can probably simplify things. Do we really need a plugin, or can we simply use an "In-Review" status to achieve the same thing? I suppose the answer to this depends on how strict you want to be.
Here’s what I’m thinking with regard to the history of a ticket (or issue, task, work item, or whatever we choose to call it):
- New
- In-Progress
- Resolved (or, if we determine that a ticket should not be completed, we have alternatives, such as deferred, rejected, duplicate, etc.)
- In-Review
- Closed
With a setup such as this, we can use the "Resolved" status as an indicator that an Issue has been completed, but it is not yet ready to be closed. Tickets are only closed when appropriate peer review actions have been taken. Who determines what these actions are? That is up to the project manager (or the team lead), and it is enforced by proper routing of the ticket. Easy individual responsible for peer review is assigned the ticket. Seeing the "In-Review" status, this colleague reviews the code changes (observing the changeset that is attached to the ticket) and makes comments (in the ticket notes).
I know this sounds like a bit of legwork, but I see a few major benefits of an approach like this:
- Tracing - We now have an audit log of all peer review comments. Using our ticket system with configuration management integration, tickets, changesets and review comments are linked together and not lost in some email thread or document somewhere.
- Time Savings – Anyone who has ever sat through a peer review (and I’m guessing most project managers and developers have) knows how insanely time consuming they can be. Because nobody ever seems to have time, we attempt to save time by doing a large review of code; We wait for a long time, and then we are faced with a peer review involving an overwhelming amount of code. This leads to the next benefit…
- Better Focus of Reviews - I don’t know about you, but I find that I am much better at reviewing a smaller amount of code or a single functional area than attempting to review thousands of lines of code all at once. We’re all busy, and this isn’t going to change. What happens when you find out that you have a peer review at the end of the week and you have to read through and mark up 5 class files? Do you set aside everything you are working on and do it? You try, but time is short, and so you hurry.
- Communication - When I take the time to review a changeset, it benefits both team and the individual performing the review. Now I am better informed about what others are working on, where it is implemented, how it is implemented, etc. I don’t have to go bug Joe the Developer to ask him if he finished such-and-such. I already know that he did because I reviewed his code.
This all assumes that our team follows good project management when it comes to the handling of issue tracking and version control. It means that we have to have well organized tickets and we have to commit changesets in some meaningful fashion. This should be a no-brainer.
