Aplikacja.info believes in continuous integration and frequent reases, so uses so-called stable trunk policy. That means: at any time any developer can make installation from trunk to production systems. How stable trunk can be achieved?
- any commit done to trunk is protected by set of automated tests that should run 100% clean (more critical project will use framework for a continuous build process like Cruise Control or PQM).
- "risky" tasks are done using peer code review process based on version control software
What to review?
We believe the most efficient method of reviewing code is to look at changesets (unified diffs). Then reviewer can see exact changes done by another developer to meet task criteria and codebase to review is minimized to really important parts of code (changes with some lines of context). That's why we believe forming proper changesets is very important requirement.
Commit methods
There are mainly three methods of inserting new commits into trunk (specified in our implementation by special "target" field in bug tracker):
- Direct commit (target=master) without code review: used mainly for simplest functionality without much risk involved. Minimal administration burden, minimal merge conflict.
- Patch Based Review (target=patch) using PMS functionality for one-way code review: in this scenario only one developer is supposed to modify patch, another one is only reviewing. At least two developers are involved, merge conflict minimized by basing on trunk (patch is always re-based on trunk during development).
- Shared branch (target=branch) using VCS: Development is branched on separate branch and reviewved on this branch. Many developers can commit on this branch/review before merge. There is higher merge conflict risk for long development on branch.
A convention used by us: patch / branch name should contain noun from task summary and task id separated by a dash:
noun-task_id
for example: invoice-2351. This way we can easily find topic branches and precisely point to taks specification (by a task id).
Peer Code Review
Where's peer code review here? Three modes of commit, three review configurations follows:
- Direct commit: changeset is reviewed after commit on trunk (as I said lower risk encourages us to be flexible)
- Patch-based (read-only) changeset view: patch is sent to another team member for review by email or shared patches repository
- Classical "topic branch": read/write review: team member switches to topic branch and checks latest commits, he may also place comments in source code
Old, Good TODOs
What is a result of code review? The most efficient way to track problems found is to insert comments inside source code (in comments, near problematic code location). We prepared a convention of well-known TODO tags in source code: TODO:nick description. "nick" is abbreviation of comment recipient, description should be short and valuable. Here's the example:
/* TODO:DCI VAT report summary should show taxes grouped by rates, not the case now */
Recipient then scans all source code for such TODOs and fix problems found (thus removes TODO in the same commit). This way:
- we can connect problem report with problem fix in one changeset
- we can track who entered comment and who fixed it (from SVN history)
Summary
Code inspections were found to be very effective method of raising quality of software (the IBM story). I believe informal implementation called peer review plays very well with other agile tools we are using for developing better software.