Skip to content

Using StarTeam for Code Review

Steve Trefethen posted some interesting stuff about the Delphi build process and then some follow-up answers to questions raised in comments. In the latter, he writes:

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:

  1. 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.
  2. 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.
  3. I expand the "out of date" tree node and look at the files which have been changed by other developers, one at a time.
  4. 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.

{ 9 } Comments

  1. Steve Trefethen | June 15, 2005 at 5:26 pm | Permalink

    Hi Craig,

    I just reconciled my build which is about 40+ hours old and there are 123 Out of date items, where should I start?

    -Steve

  2. Craig Stuntz | June 15, 2005 at 8:40 pm | Permalink

    Presumably with the files which are in your area of responsibility. You don’t have to use "All Files by Status" as I do (I work on a smaller team!). You can set up a custom filter on either files *or* change requests and go through the list using the filtered view. You can then just accept the files that are out of your scope to review (i.e., which don’t show up in your review filter). The filter might restrict by name, location, file type, revision label, etc. Presuming you coordinate reviews with other folks on the team, you can still be sure that all checkins get a second (third, etc.) set of eyes without a single person (or, worse, everyone on the team) having to do *all* of them.

    That’s part of the reason I want to add a "Ready for code review" status for change requests. In addition to allowing us to not mark the CR as "finished" until it is both implemented and reviewed, it would make it easy to coordinate multiple reviewers.

    Since this is non-obvious for folks who aren’t used to StarTeam, I’ll give more details, although I suspect Steve doesn’t need it. When you check in a file in StarTeam you have the ability to bind the revision to a change request, and the project administrator can require this. The change requests are also versioned, and the links can be pinned to both file and CR versions. You can then go through all of the change requests with a certain status and see which files linked to the CR revision where the status was changed to "ready for code review." Once you have examined these files you can change the CR status to "fixed." At this point StarTeam will automatically fill in the "fixed in version" field the next time a build label is applied to the view.

  3. Shawn Oster | June 15, 2005 at 10:02 pm | Permalink

    Interesting, I do something on a much more informal basis. We use Subversion and I ask to see all files that have modified since I list did an update and then look at the log of files and their comments. I also do a diff of changes and go through it, though I’ll sometimes actually get that version then revert it so I can have syntax highlighting or the ability to jump around quickly in code if a lot has changed.

    I like the idea of making it part of the formal process though. Subversion has custom attributes you can add so perhaps I’ll stuff a change request item in there.

    Thanks for the ideas!

  4. Erwien Saputra | June 16, 2005 at 12:11 am | Permalink

    Hi Craig,

    You take too much responsibility on yourself, by reviewing each changes. In many cases, a developer may be an expert in some area, which makes him better reviewer than other developers.

    You approach means that the code is reviewed after being checked in, if the reviewer found something may go wrong, the code may be check out and check back in multiple times. Too much unecessary revisions make revision history less effective.

    It can be worse if a team does not have coding standard, or someone decides to reformat the whole unit. comparing the content between revision or with your local copy can be a challenging task.

    IMO, code review should be done prior checking-in the code.

  5. Craig Stuntz | June 16, 2005 at 8:53 am | Permalink

    Erwien wrote:

    "Too much unecessary revisions make revision history less effective."

    This is perhaps true with old-fashoned systems which only do source control. But it’s also impractical — if it takes me six weeks to implement a feature, I don’t want to hold off on checkins during that time period. And if a "fix" fails testing, then you *must* create new revisions. Still, I know what you mean from having used VSS histories.

    With a full-fledged SCM tool like StarTeam, however, it’s a non-issue. Remember, all revisions are bound to change requests. If it takes 15 revisions to implement a feature, that’s fine — I can see them as a group.

    In fact, I’ve asked folks on my team to start creating *more* revisions. If there are three bugs in a unit, for example, I’ve asked that each fix get a separate revision. This makes the revision<->CR relationship much more explicit. When I want to go back in time and see why a code change was made, I don’t just rely on the checkin comment. I can see the change request, test results, etc.

  6. Marc Rohloff | June 16, 2005 at 1:51 pm | Permalink

    Craig,

    What are you using for your CRs / requirements gathering ?

  7. Craig Stuntz | June 16, 2005 at 2:31 pm | Permalink

    Marc, we use StarTeam for defect tracking and feature requests, and CaliberRM for requirements. We just completed the transition away from Rational RequisitePro, which I am very happy to leave behind.

  8. Morten | August 14, 2012 at 5:02 pm | Permalink

    My main problem with this approach is that code that failed a code-review still got checked in. It shouldn’t have gotten checked in in the first place.

  9. Vel | May 29, 2014 at 1:35 am | Permalink

    By this process you will not have a stable branch as the codes are checked-in before review. In perforce changes can be reviewed before the actual commit whereas in Starteam we dont have a concept of pending changelist, so whether the code is right or wrong we need to check-in then review has to happen in Starteam which is a flaw.

    By the way in our organisation we use Starteam for version control and issue tracking. It will be great if starteam offers pre-commit review option.

Post a Comment

Your email is never published nor shared. Required fields are marked *

Bad Behavior has blocked 713 access attempts in the last 7 days.

Close