 |
Intro: Every company in the tech space that I’ve worked for has had a QA dept. However, it wasn’t until I worked with Troy Waldrep at Pervasive that I heard about code review. The next time I heard about it was from Jason Cohen (blog), founder of Smart Bear Software, whose company offers a tool called Code Collaborator. Recently, I’ve been increasingly interested in software quality and process. So, I decided to ask Jason to talk to me about code review, and its role in the software lifecycle. |
Lynn Bender: I recently asked a head developer at a software company if his team had a formal code review process. He immediately got defensive and replied: “yes”. When I asked him to elaborate, he began to explain their QA process. So to begin, what is code review?
Jason Cohen: Everyone’s had to write a page of prose for school or work. Have you ever done it without making any mistakes? I’m not just talking typos — what about points that could have been clearer, paragraphs that didn’t make sense, missing segues, etc..
Bender: As a matter of fact, I pretty much always pass my important business communications by a colleague to read before sending out. It’s amazing what I miss.
Cohen: Of course you can’t do it, which is why most everything in the publishing industry is reviewed by editors. It’s not because writers are careless or incompetent. It’s because with any sort of creative knowledge work we’re “too close” to our work to see the problems. It’s normal and unavoidable.
So if no one — not even top-notch professional writers — can write a page without needing an editor, why do we expect software developers to write code alone?
That’s all a code review is: reviewing someone’s work.
Note that, as in writing, often the errors are things the author would have seen had the author been reviewing someone else’s code. It’s the “you’re too close” problem most of the time, not the “you’re incompetent” problem.
Bender: This would seem to have a larger scope than that of QA — encompassing programming style and method.
Cohen: People often think of code review as a process to “find bugs.” Although this is an important result — the one management is perhaps most interested in — there’s many other purposes. Finding maintainability issues (e.g. documentation, organization, architecture) saves time for the next developer and prevents bugs in the future. Sharing knowledge about programming in general and about gotchas in the code base helps everyone write better code. Overseeing new hires means noobs can get their hands in the code as fast as possible while ensuring they don’t break anything.
Bender: Can you tell me a bit about what the code review process looks like, and how does it fit into the overall dev process?
Cohen: There are many types of code review, all of which are useful and have trade offs. At one extreme you have the over-the-shoulder walk-through, performed whenever the author is uncomfortable about their work. On the other extreme is the Formal Inspection where participants must be trained, a seven-phase process requires four separate meetings, and reviews take two man-days to complete.
Typically the less formal processes have a better chance of succeeding, unless you’re a CMMI-Level-5 DoD contractor or some such and your employees don’t mind (or are used to) paper-pushing.
Code review comes after the code is written but before QA. Some like doing code reviews before code is even checked into version control, so “checked-in” also means “reviewed.” Others say that causes too much developer down-time waiting for the review and therefore prefer reviewing just after check-in. There’s no right answer to that one.
Bender: Is there much difference in how one would implement code review for an agile team?
Cohen: No. I don’t think e.g. Scrum means you don’t need to design, architect, review code, or do QA, it just means you need to do it in a compressed time-frame.
The exception might be pair-programming, which you could argue is an in-depth, continuous code review situation. In fact, I sometime argue that other code review techniques get you some (not all!) of the benefit of pair-programming at a small fraction of the time, and is therefore a better time/result trade-off for people who don’t want to go all the way.
Bender: Where is the optimal place for code review in the life cycle of a project?
Cohen: Clearly at the end of the lifecycle, during “feature freeze,” code review makes a lot of sense. By definition you’re trying to be extra careful about every check-in. In fact, in past companies we used to self-impose code review during those times.
However it’s just as useful earlier in the cycle. If you only review at the end, mistakes made long ago might be too embedded for change. Sort of the same problem as waterfall development!
If you’re religious with unit tests and constant refactoring, then it may not make sense to code review early on because it’s so safe to change later. In practice, few development groups are zealous enough for later major restructuring to truly be safe and fast.
Bender: With so many startups in Austin, we’ve had the opportunity to watch many software shops go from one or two people to a group of ten or twenty developers. At some point, most of these groups implement a bug tracking process, and half will have a formal QA process. However, I am surprised how few actually implement code review as part of the dev process. Has this been your experience as well? When is the right stage to begin a code review process? And for companies that already have a few hundred thousand lines of code, where do they begin?
Cohen: You’re right that code reviews are often the last quality process to be introduced, perhaps because it’s so social.
If a team is not already doing code reviews, I would strongly suggest reviewing only a little bit of the code. For example, have optional reviews, done just when the author is unsure, or just on a stable code branch, or just on the 10 files voted “scariest to change.” Stuff that everyone would agree is worth some extra effort. Going from zero review to reviewing everything is too much of a change to do at once.
If you have a large code-base, you could pick out files you know are trouble and review them first. Or, just do incremental reviews — reviewing each check-in — and find the problems as you go. If you do this you’ll naturally notice problems around the code being changed — even in code that wasn’t relevant to that check-in — and you can fix it then.
Bender: Your company, Smart Bear, is widely known for it’s code review tool called Code Collaborator. Can you tell me a bit how it facilitates the code review process?
Cohen: The most important part of the review — human beings thinking critically about code — is something no tool can help with! However there’s also lots of mundane aspects of code review where a tool can help, and that’s what Code Collaborator tries to do.
First, Code Collaborator integrates with version control so with a single click you can do things like “create a review with all the files I’ve modified but not yet checked in.” It can grab the files along with the content as it appears in version control and make diffs.
Second, Code Collaborator displays diffs together with chat in a web browser. The chat is live (or works like a newsgroup if you’re separated by lots of timezones), and is threaded by the line of code. This saves you from tedious notes like “on line 723 of //depot/project/foo/bar…” and emails with quote-carets all over the place as you try to make sense of everyone talking.
Third, Code Collaborator automatically collects metrics like time spent in review, number of lines of code, and number of defects found, so managers get the reports, audit trails, and numbers they need while developers don’t have to lift a finger. No forms, no stopwatches, no meetings, no rubrics.
Bender: I sometimes get this comment from developer friends: Because the dev environment in most startups tends to be fast paced, implementing processes like code review are often viewed as a “frill”. How would a developer best make a case for implementing code review?
Cohen: If you’re in an early stage of development, you might not be able to make the case! Early on, just getting something out the door is more important than bugs.
At some point though, your company turns the corner and keeping customers happy and having a solid platform becomes more important than the number of features you can spit out per quarter. It’s at this time — when you start talking about static code analysis, QA departments, integration tests, etc — when code review should be part of the discussion.
I think the best argument is a combination of common sense and measurement. The common sense part is the author/editor argument I gave earlier. It just doesn’t make sense that anyone could write perfect code in a vacuum, and it’s just as obvious that another pair of eyes could help. Just as spell-checkers are useful but not a substitute for a personal review, static code analysis is useful but not sufficient.
The other side is measurement. Code review is something you can prove has value! Just measure the number and type of defect you’re finding and the amount of time you’re spending in review. Then just divide and see if it’s worth it! The usual number is that you find AND FIX a defect every 10-15 minutes. So ask yourself, can you do that with QA? Shouldn’t you at least try to do it with review?
Bender: That’s a strong case. Jason, thanks for taking the time.
You can read more of Jason Cohen’s writing on software at his blog: A Smart Bear.
