Recently I’ve been thinking about our code review process, and how it could be improved. We’ve recently moved from a simple VSS based source control to a more feature rich TFS based approach.
We now have the majority of feature development occurring in several development branches, at the end of each development iteration we get all the changes reviewed and then merge the changes into our Main (or trunk); a build from Main will then be passed to test.
Problems we’re finding with this approach are:
Development Code Freeze
Whilst code is being reviewed (and review actions being actioned) we pretty much freeze feature development in the branch, the code review can often take a long time, so people often start doing further development off-line, ready to check-in once the freeze is lifted.
How can we make sure that what is eventually merged into Main is what was reviewed?
If the review is done against the changes between Dev and Main then you miss reviewing any merge conflict resolutions (unless you do a 2nd review). If you review the ‘pending merge’ (where someone does all the merge process apart from the final check-in to Main), you risk the merge no longer being valid (someone else checks into Main between pending merge and review complete).
Seeing very little benefit from the review
Our process of reviewing all code going into Main is giving us a false sense of confidence, as we think Main has top-quality code in it; in fact this is not the case, often it seems the only review comments that are actioned are fairly trivial changes that wont take very long. Any fairly major changes are often deferred to a future ‘Change Request’, with the merge going ahead anyway, as the feature is required.
We do want to prevent breaking changes going into production or even to test, but this should probably be caught by testing (automated) and Continuous Integration.
Code review is inconsistent
Code reviews are processed in a fairly inconsistent out-of-band way, with very little formal process around it. Review comments can sometimes be lost, and tracking whether the review was fully actioned etc is not 100% possible.
Suggestion: Do the code review after the check-in to main
So the suggestion I’ve been making is that instead of holding up the check-in to main with the code review we do the code review afterwards. The process would be something like this:
- Feature development is done in a development branch
- Towards the end of the iteration the code is prepared for check-in to main
- The code is merged into main
- The check-in to main causes a code review task to be created for the changeset
- Code review actions are done directly against main.
- Once the code review has all been resolved (actioned, further tasks raised etc). It is marked as closed.
- Before main is pushed to production all feature development code reviews must have been completed.
- Development Freeze – by decoupling review from development the two activities could run in parallel, as soon as the merge is complete development could resume. If merging up to a label you wouldn’t even have to freeze whilst the merge is done.
- Review exactly what was merged – by definition we would be reviewing what was merged. We could even say that review actions should be reviewed so no changes to main go unreviewed.
- Review benefit – by decoupling review from development and making the review process more traceable I think we will be able to add review actions to the product back-log and then even the big changes will be done. Review actions should be treated in a very similar way as feature development with priority dictating what is actually done.
- Review inconsistency – having a proper review process that is tracked with tasks will means a more consistent approach and a reliable process. Any unresolved review comments will be searchable/reportable just like any other tasks.
- I think we would risk checking in bad code to main, and although we could use our source control system to roll these back etc, we might not catch them in time – I don’t think, however, that our current process really stops this happening.
- Reviewing could be neglected in favour of feature development, but this just means that project management needs to see the importance of the review process and prioritise the tasks appropriately.
Accepted Best Practice
Before writing this post I tried to search for the best practice in this area and couldn’t really find any. I guess one answer is that instead of code reviewing paired programming could be more effective. Does any one have any suggestions as to what others are doing?