Code Reviews
A recent post on Embedded.com about Peer Code Reviews got me thinking about the process we use in the LabVIEW team for checking in code. It came about many many years ago and is, in my opinion, the most effective process I've seen. We call it "buddying", but a lightweight peer code review is probably the technical term.It can be summarized quickly as: No code gets checked into the main branch unless the changes have been reviewed by a second person.
The practical process is
1) The programmer makes the code changes. These changes may be checked in to a private branch in source control or may be just sitting on his/her hard drive.
2) If the changes are in the 100-line or fewer area (which is a majority of submissions), the programmer goes and "gets a buddy". The buddy is typically a senior level programmer who is already familiar with that code area but that's not a hard rule.
3) The author walks through each line of the changes with the buddy and explains the changes. The buddy points out any stylistic problems, double checks for off-by-zero or null pointer handling code, and reminds the author about any gotchas that may be in that area.
4) If the buddy is satisfied with the code, it gets checked in to the source code control system and the author & buddy's names are noted.
That's it. It adds about 10 minutes to each code submission and keeps a lot of "doh!" errors out of the code. If you don't have a system like this now, I recommend you try it. We find it invaluable.
Labels: LabVIEW
posted by Joel @ 8:32 AM
4 Comments:
Your blog is very interesting!
Bye bye
As the development team built all in one, I have a "similar" approach:
1. As the developer I make the changes.
2. As the project manager I allocate my time elsewhere for a week or so.
3. As the architect I review the changes and document my testing procedure for that particular submission.
4. Retest.
5. Pray I didn't miss anything.
What source control tool are you using? Accurev has a nice promotion model for this kind of thing built-in. Developers can 'keep' a file until ready to promote or check in to the dev stream, or a code review stream can be easily created/inserted between two dev streams (QA and integration for e.g.)
NI uses Perforce and find it quite good. In fact, I just went to their site and our own Steve Lysohir is the front page testimonial. They have a case study for NI's usage on their web site. The case study has info on all of the reasons we like Perforce. The cross platform issue does seem to get relegated to the "environment" column rather than in the main text but that is rather critical I'd say.
Post a Comment
<< Home