I have been working with a code being maintained for years. We're using clear-case as version control. To submit the code for review and get the code count (for project metrics) are usually painful.

What's the best way to conduct code review for maintenance software? How effective we can share the code for peer review (I understand it's tightly coupled with the version controls we're using). Any known standards or best practices? How can I get metrics like code count etc.?


Code review process

The most successful way that I currently perform code review is the following.

  1. Create a branch/fork of trunk and label it against your ticket number
  2. Have your developer check in all code against that branch
  3. When time to perform code review a senior developer or peer merges the branch against their current working copy of trunk and then performs a code review.
  4. If the review passes and the branch meets quality and the ticketed issue the changes are merged with trunk.
  5. Otherwise issues are flagged with the original developer in a 1 on 1 discussion and the ticket is either place back the list for rework and review or is dropped from the release if there is not enough time for rework.

Metrics don't work the way you think they do

Capturing "lines of code" type of metrics is a common Anti-Pattern, and should have no real place in the code review process.

There are great reasons why some kinds of metrics simply don't work. Joel Spolsky has written some great articles about metrics and metrics anti-patterns, and it really comes down to what outcome you wish to have from your metrics.

In regards to metrics, a great developer can write 1 line of code that performs faster and is more efficient than a mediocre developer who writes 20 lines of code to do the same thing.

Metrics you should be interested in other than evidence based scheduling is metrics regarding code quality. Packages such as NCover run over your daily build, and so these work quite well with the above code review process.

| improve this answer | |
  • Actually, in a code review, lines of code could be a useful measurement. If you record the number of lines read prior to a meeting and lines reviewed during the meeting, along with time spent reading the code before and time spent during the meeting, you can try to gauge meeting effectiveness. Lines of code aren't a good measurement of a productivity metric (LOC/hour, as an example, isn't so great). – Thomas Owens Jul 27 '11 at 12:12
  • @Thomas Owens - During code review I try to get developers to refactor their solutions to be elegant, reduce cyclomatic complexity, increase abstraction & readability. This usually results in a significant reduction in LOC in the final solution. I'm not sure I follow how LOC can be used to gague meeting effectiveness? – Justin Shield Jul 27 '11 at 12:58
  • That has no effect on the number of lines of code that were read by reviewers before the code review, the number of lines of code read by the group during the review itself, the time spent by an individual developer reading the code under review prior to the meeting and annotating that code, and the total time spent during the meeting. – Thomas Owens Jul 27 '11 at 13:00

The easiest way to conduct code reviews is over-the-shoulder reviews. When you are ready to commit your code you ask for a colleague to come over and review your code with you. Then when you commit your colleague adds a signature of sorts to the commit message to indicate that they have reviewed the code and approved it. This is very lightweight - it doesn't generate much overhead. The downsides are:

  • One of your colleagues has to be available to perform the code review, in person
  • They have to drop what they are doing to review your code, or you have to wait with your commit
  • It's easy to "fake" the signature
  • There is no paper trail of the code review unless you also take notes and attach them to the commit somehow.

This, to me, is "the simplest thing that could possibly work" for code review.

| improve this answer | |
  • Given they use Clearcase, it's probably too simple to keep the bean counters happy. – mattnz Jul 27 '11 at 10:24

Code reviews are worthless unless you review committed code (changes can and will be made between review and commit). Reviews of more than 20 or 30 lines are dubious at best, meaning that the process has to be light weight. However (given your into Clear case and concerned about metrics I suspect you fit in here) for many business the measurements are more important than actual results, so you have to be able to show you did it by measureing something, and it's hard to get meaning measurements from reviews.

Look at how GIT and GERRIT work together, they seem to provide a good balance.

| improve this answer | |
  • It's not hard to get meaninful measurements or metrics from reviews. LOC read prior to meeting/time, LOC reviewed/time, defect removal effectiveness (although you would also need requirements reviews and design reviews, along with testing, to get a full picture)... – Thomas Owens Jul 27 '11 at 12:14
  • @Thomas It's a given that Peer review is a low-cost (the lowest) way to reduce defects. Certainly the things you have mentioned are measurements, how are they meaningful, as in "improve the end product compared to if they were not measured"? The only measurement I have seen that makes sense in real life is "Was a peer review performed". If you can provide evidence that these measurements make a real difference and have a cost benefit basis to the organization, please do so as I am more than happy to be proved wrong. – mattnz Jul 27 '11 at 22:50
  • It's not necessarily about how they improve the end product just because they were measured. Developing a quality process is just as important as developing a quality product. If you spent 8-10 man-hours a week doing code reviews, but 75% of defects escape to integration testing, you're, quite simply, doing something wrong. Code reviews are proven to improve quality of code, but if you are spending adequate time, yet not seeing benefits, it's important to find out why. I would recommend reading Metrics and Models by Kan for more about quality engineering at the project and process level. – Thomas Owens Jul 27 '11 at 23:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.