Steve, do you do code review? How do you do it?
Yes, although like most teams I suspect we could be doing better in this area and in fact I have a number of things I plan on blogging about related to this area so stay tuned.
I have some humble advice for this: Use StarTeam. This product has really revolutionized how I do code reviews. Perhaps other source control tools can offer related features, but StarTeam’s integration of change requests with file versions and ability to quickly update the status of thousands of files at a time allows me to do things that I could never attempt when we used Visual SourceSafe.
Here’s how I do code reviews with StarTeam:
- At the beginning of the day, and periodically throughout the day, I run the StarTeam client, select the Files tab, and navigate to the top folder of our source code.
- Then I press the "All Descendants" button on the toolbar, which flattens the folder hierarchy into a single list of every source file in our project. I’m using the default "All files by status" filter, so the files are organized by whether or not the version presently on my local drive is current, out of date, or changed by me.
- I expand the "out of date" tree node and look at the files which have been changed by other developers, one at a time.
- For each file, I click the "History" and "Links" tab. This shows me immediately how many checkins have occurred since I last got the file, what the check in comments were, and which change request is linked to the checkin. Note that you can mandate both check in comments and process items (such as change requests) at the time of check in in your project options. Then, I use the Compare Contents tool to show me the changed lines in each revision older than the one already on my machine. If the changes are acceptable, I check out the file without locking it (this is called "Get Latest Version" in some source control systems). If the changes aren’t acceptable, I don’t get the file and I send email to the developer who checked them in.
This means that I never get any version which doesn’t pass my review, and I see every line of code which was changed by someone else in my office. This has made a big difference in quality for our latest release; we’re getting fewer items failed in testing.
Prior to using StarTeam I had to depend on developers to ask me to review their code, since there was no way to get file statuses and change requests linked together for the entire project using SourceSafe and our spawn-of-satan requirements tool, Requisite Pro (thankfully, we have now retired both of these). In practice this almost never happened. Now I see every line of code which changes in our applications. I’ve literally gone from reviewing code only in limited circumstances to reviewing every piece of work which is done in the project.
I’m trying to come up with a good way to do this for database metadata as well. Another improvement I want to make in the process is to use StarTeam Enterprise’s custom attributes to make a "Needs Code Review" status for process items, so that they won’t get set to "Fixed" until after the review is complete, rather than immediately after implementation.