The Joel on Software Discussion Group (CLOSED)

A place to discuss Joel on Software. Now closed.

This community works best when people use their real names. Please register for a free account.

Other Groups:
Joel on Software
Business of Software
Design of Software (CLOSED)
.NET Questions (CLOSED)
TechInterview.org
CityDesk
FogBugz
Fog Creek Copilot


The Old Forum


Your hosts:
Albert D. Kallal
Li-Fan Chen
Stephen Jones

Code reviews

Hi all,

today I had an interesting discussion with a customer about code reviews. We both agreed that code reviews are beneficial.

However, while she claimed that code reviews are a standard industry practice, widley used in commercial software development, I questioned that statement, believing that code reviews are less often done in practice.

Does anyone have a hint to a reference where this topic is explored with measuring data of some sort? Again, I believe in the value of code reviews. I just would like to know how often this is done in reality in the SW industry.

Thanks for any hints,
Bernd
Bernd
Wednesday, March 28, 2007
 
 
I don't know anyone who actually does them but I do work with some guys who used to do them and are negative towards them.

Someone had an awesome comment about CR's on here a couple weeks ago - something about how CR's put a very discomfiting magnifying glass on your development process.
Greg Send private email
Wednesday, March 28, 2007
 
 
They are hit and miss.  We did public code reviews where the whole group looked at the code as a group and it was a very negative experience. We now do private reviews, mostly just sr developers reviewing the bugfixes from the more jr guys, separately and privately with feedback via email. 

It's definitely not a common or widespread practice as far as I can tell.
Sassy Send private email
Wednesday, March 28, 2007
 
 
"Does anyone have a hint to a reference where this topic is explored with measuring data of some sort?"

Try the Software Engineering Institute based out of Carnegie Mellon University at...

http://www.sei.cmu.edu/

I'm sure there are others, but I know about these places because they're the ones that introduced the world to the horror that is the Capability Maturity Model, and they did collect a lot of statistical data about software projects (generally large Defense and Aerospace projects). They may have some white papers on the benefits of code reviews, or be able to steer you towards a better resource.
TheDavid
Wednesday, March 28, 2007
 
 
Every place I have ever worked has claimed to do code reviews. Nowhere that I have ever worked has done them consistantly or effectively.
Jeff Dutky Send private email
Wednesday, March 28, 2007
 
 
It's nowhere near "standard industry practice".

Does Fog Creek even do them?

But we *do* do them now at my current job, and it works very well.  I highly recommend it.

I think it can substitute for a lot of heavy-handed management -- peer review rather than "change request" meetings and such.
Moosebumps Send private email
Wednesday, March 28, 2007
 
 
A what?
SumoRunner
Wednesday, March 28, 2007
 
 
At one job all code was reviewed by the development manager, line by line.  He would even print out the code and track variable lifetimes with colored pencils.  He was like a human shaped debugger.  It was actually incredibly effective.

Since then, though, the only real code reviews have been ad-hoc ("can you check this over?", "I noticed you checked in fix A, did you know about neato language/construct/feature/pattern C?").


I think to be successful you need people who can take criticism and are willing&able to learn from the review -- and realize that everyone (sr & junior) has something to offer.  Without a .. nurturing .. (heh) environment though, I can see how it could turn into an excuse to eviscerate a hated co-worker.
Dan Fleet Send private email
Wednesday, March 28, 2007
 
 
I agree with the OP that reviews are valuable, useful, huge bang for buck, and... very very rarely done in US tech industry.
Meghraj Reddy
Wednesday, March 28, 2007
 
 
Never ever seen code reviews. Work in mom&pop and Fortune 100. I've seen a lot of unsecure code written by by manager/developer and other developers.
curdDeveloper
Wednesday, March 28, 2007
 
 
I've heard rumors they exist, but in 20 years I've never seen one actually take place.

We're more likely to see Sasquatch than take part in a code review.
Anon
Wednesday, March 28, 2007
 
 
It's definitely not standard industry practice. It might be standard in medical devices, DOD or similar projects with high criticality, but that's about it.

When I've been involved with code reviews that were **effectively managed** worked great and we got a lot of improvements done. What I mean by "effectively managed" is the following:
1) Nothing is taken personally, and the review isn't used for personal attacks
2) Everyone has a checklist of items to look over
3) The meetings are quick and well organized
4) Everyone takes ownership
QADude Send private email
Wednesday, March 28, 2007
 
 
At my workplace developers regularly send each other code to review when the developer who wrote it is unsure of its correctness/quality or simply wants it reviewed.

Also, to make changes to the release branch, the patch *must* be removed.

Group reviews are another story though. In theory they sound nice but I've never had good experiences with them. One always ends up with a group of pedants arguing about naming conventions and spacing issues.

Also, at the first company I ever worked at -- a tiny startup -- there was one 'senior' guy who would use code reviews as an opportunity to show how much smarter he was than everyone else. He'd review code and then send an email to every developer in the company pulling the code apart piece by piece. Needless to say, that wasn't good for morale.
Kaln
Wednesday, March 28, 2007
 
 
What's a code review?

Wednesday, March 28, 2007
 
 
> What's a code review?

A good way to waste time.
Jussi Jumppanen
Wednesday, March 28, 2007
 
 
> A good way to waste time.

It's a good way to create quality code. You'll find that just the idea that people will look at your code ups your game a lot.

And no, code reviews in very few places at all. But the only industry standard practice I an aware of is to have no standard practice.
son of parnas
Thursday, March 29, 2007
 
 
Never seen it happen in a meaningful way. On each project we're always too busy dealing with the bugs caused by not having code reviews to do code reviews.
Codeless
Thursday, March 29, 2007
 
 
Code reviews are a standard practice as part of a formal development model.  They are required by some military contracts.

Implementing code reviews is tricky.  One on one sessions are proven to be as effective as large, formal reviews.

Additionally, assigning each reviewer a specific subset of issues to look at (responsibilty) increase focus, coverage, and reliability.  Makes them much less of a time waster.

If your going to do something, do it right.

Thursday, March 29, 2007
 
 
I've only been at three companies.  At my first company, a small 30-person startup, we did code reviews religiously.  At my next company, a floundering .com bubble-era 500-person enterprise, we didn't (despite my efforts to bring the practice into my workgroup).  Now, at my corner of Microsoft, we have a code review for every submission.

My opinion of them is mixed.  In the initial stages of team formation, they're immensely useful in swapping code tricks and standardizing on a common style.  They're also useful in getting college hires and other newbies up to speed in "real world" coding.  But in a mature group where everyone knows each other and most everyone programs the same way -- such as the group I'm in now -- code review comments rarely contain anything of major significance.  I'd rather forego them and spend more time on large scale code inspections and design reviews.
Alyosha` Send private email
Thursday, March 29, 2007
 
 
To be more specific, "code review" refers to the practice of having at least one person sign off on a bugfix or small code change / feature add.

In the initial stages of implementation of a large chunk of code (assuming everyone's on board with the general design), the review requirement is usually waived in light of the fluid stage of coding, where everything is understood to be in flux.  Sometimes we follow this up with a large scale code review, although this usually means "a very superficial review of the code done in a one-hour meeting full of people who've never seen the code before".  I try to push for more rigorous forms of code inspections (budgeting 1 hr per 200 loc, in a room full of people who have read the code, in a process streamlined for logging issues and resolving them offline).  I've met with mixed success.

But many of the time, the code just goes right in ...
Alyosha` Send private email
Thursday, March 29, 2007
 
 
I've done code reviews for most of my software development career. I think they're great _when_ they're done properly. I suspect most of you who don't like them have had a bad experience with them.

The stance I usually take on code reviews is _not_ to sit there and pick apart other people's code. Instead, I want the author of the code to explain to me what their code changes do and why they've done them. The reviewee does most of the talking. If they can convince me that the change is justified, then that's fine.

I will occasionally ask questions about certain areas, such as "What happens if that function returns null?" or "What happens if this loop has to iterate over a million items?", but that's just a front to getting the reviewee to talk a bit more about their work, and to show me they've thought about those things.

I've usually done code reviews against peers. It's rare to get a manager involved because they don't know enough technically to be a good judge of the code's quality. Having said that, I have had some good managers who I can rely on being savvy enough to be a good reviewer.

Most code reviews take no more than a few minutes, the exceptions being brand new features which take a bit longer.

What I've discovered when doing this, is that most problems found during the review are found by the author of the code, not the reviewer. It seems that just by talking about your code to somebody else, and explaining it, prompts you to think carefully about your code and look at problems you might have missed the first time.

Now I think it's fair to say that most people are _much_ happier when they find a bug in their own code but somebody else didn't, than when somebody finds a bug in their code and they didn't!

The other benefit of code reviews is that if you can explain your code to somebody else and they understand what you're doing, you can take one step away from the nasty situation where you pick up somebody else's code (due to them leaving the company, getting run over by a bus, agreeing to appear on the next series of "The Apprentice", and so on...) you're not sitting there scratching your head as to why this pile of WTFs got in the code base.

A review is supposed to add quality to your code. If a review doesn't add quality, don't have the review! (Or change the way the review's done).
Ritchie Swann Send private email
Thursday, March 29, 2007
 
 
We have code reviews. But they are very far between. And I think it is great for someone that is more experienced than you to review your code. But now that someone is gone and now a co worker that is as experienced as much as I am is doing the reviews. And I find that de moralizing.

And we don't do any mission critical applications. Just your usual boring enter numbers and store them in DB type of apps.
Jan Hančič Send private email
Thursday, March 29, 2007
 
 
At Exoweb each commit has to be reviewed by three mates. I am new here, but it seems pretty effective until now.
Ionuţ Bizău
Thursday, March 29, 2007
 
 
If you are not performing manual peer inspection of your code, it is broken.

I have yet to see a non-trivial code review (1000+ lines) that did not find at least one flaw.
UNIX Guy
Thursday, March 29, 2007
 
 
Code Reviews are part of the standard development process at my largest customer. 

If you can make sure they are about defects and not style ("Fagan Style Inspections") then they can have some benefit.

Human nature is to either to so cursory an inspection as to have no value, or to argue about where to put the curly braces.

Jerry Weinberg has a book on Inspections -

http://www.amazon.com/Handbook-Walkthroughs-Inspections-Technical-Reviews/dp/0932633196/ref=sr_1_1/104-8199493-8403907?ie=UTF8&s=books&qid=1175172036&sr=8-1

I haven't read it, but I literally have such faith in Weinberg that I would recommend it without reading it.  Jerry has something like forty books on software development, and I have read maybe seven or eight of them.

The key problem I see is to get the reviewers to actually invest the time into finding the defects and grokking the code _before_ the meeting.  If everyone else is under a deadline to get their own stuff done, why would they waste time inspecting your stuff?

One response to this is to create a class of people who don't *do* anything, they just *review*.  You might call them "Architects."

I think that approach is crap.  It results in a class of people who are very highly paid but don't produce anything.

One team I worked with measured the number of code reviews done by developers, made it a graph and put it on the wall.  That's okay ... but I think it would encourage people to do lots of cursory code reviews.  In other words "I need a code review", "Go ask Aaron - he'll get it done in ten minutes and won't find anything ..."
Matthew Heusser Send private email
Thursday, March 29, 2007
 
 
Matt:  The answer, like I previously mentioned, is to assign people tasks (e.g. Bob, review the code for memory issues.  Steve, can you look to make sure the code is multi-treading safe).

If you want to go even further, the categories should be standardized.
Anti-social
Thursday, March 29, 2007
 
 
I work in medical devices and we code review everything. Some of the reveiws are good (catch subtle bugs like public inheritance from an STL container), some are horribly tedious ("why are/aren't you using an STL algorithm to iterate over the values in the container"). It all depends on who attends. In my experience, they tend to be "good" for catching local problems, but start to fall apart when applied at a larger and larger scale (who has time to review the entire context in thich the code under review fits?).
A. Nonymous
Thursday, March 29, 2007
 
 
Matthew, are you suggesting that code reviews need to be done in a meeting? I find they work best in as informal situation as possible, and reviewing small changes. You grab the best person available to review your work and run through it.

Since I tend to commit new code in small packages (more than three days between commits for me is rare), the body of work under review is never particularly great.

One potential flaw with that is knowing who the "best person" is. If you're on a team with several developers on the same project, it's a pretty good idea to pick one of the other team members. If it's somebody who has to use your code at a later date, even better.

If your work tends to only have one developer per project, then knowing who's best is a bit trickier. But even then, the act of reviewing code on other projects is good for propogating ideas across, so you don't get everybody solving the same problem in 5 different ways.
Ritchie Swann Send private email
Thursday, March 29, 2007
 
 
Don't start doing code reviews unless you can do them once a week or more for each developer in the beginning.  If you let someone get four weeks into a three-month project before you look at it and tell him the design is unmaintainable crap, reality will intrude, you'll test it to make sure that it works "well enough", and tell him how to do better next time.
Drew K
Thursday, March 29, 2007
 
 
"If you let someone get four weeks into a three-month project before you look at it and tell him the design is unmaintainable crap..."

That sounds more like a design review than a code review.
A. Nonymous
Thursday, March 29, 2007
 
 
Are there places that do design reviews but not code reviews?  If you're talking about starting code reviews for the first time, I'm guessing they haven't had design reviews either.
Drew K
Thursday, March 29, 2007
 
 
at my first job, we did code reviews as part of our "education".  several of the developers were of the "self-taught" type and our coding practices were not always the most efficient.

no fingers were ever pointed.  we rotated on who's code we were reviewing each week.

i've never worked anywhere where code reviews were done as part of the software life cycle, though.
Kenny
Thursday, March 29, 2007
 
 
I've never been part of a code review.....and I am NOT an advocate of them.

To me, a code review quite simply is the equivalent to MICRO-MANAGING.....look, you either trust me or you don't to be competent enough to write code. If you trust me, don't micro-manage me with some bullshit code review.

If you don't trust me, then find someone else to develop your app.

If my code breaks, I will be the FIRST one there to repair it. I work with small-scale solutions so code reviews are virtually non-existent. This is one of the reasons I am not interested in working on a larger project - because I don't want my code to be excessively scrutinized....

My code runs, it compiles and it works....that's all the boss or management needs to worry about....
Brice Richard Send private email
Thursday, March 29, 2007
 
 
No, your boss cares about total cost of development.  Code reviews are a technique proven to reduce that cost.  But hey, just never apply to work where I do.

Thursday, March 29, 2007
 
 
> you either trust me or you don't

Let's say you make a 1000 choices in any one checkin. Just one mistake in 1000 could be serious. You wan't me to trust that you won't make one mistake in 1000 choices? That's absurd. So get off your high horse and realize it's not about you. It's about the system.
son of parnas
Thursday, March 29, 2007
 
 
I know of a couple mobile phone companies that do code reviews. I know of a company that make Telecom equipment that swears by them.
AChater
Friday, March 30, 2007
 
 
Brice - do you work in an IT department or make internal software as a cost center, as opposed to an ISV that makes shrinkwrap software?

If so, I can see why you don't see much value in code reviews. You might be in a situation where you can jump to it and turn around a release to your customer in no time at all. In which case, lucky you. In a MicroISV environment, if you can catch a bug at code review time before testing, or worse, shipping, you've saved yourself a load of time. For embedded software, it's even more important as the cost of fixing after you ship can be enormously expensive (who wants to have to recall 10,000 units just because one EPROM's got buggy software on it?)

In fact, even if you catch it at testing, you can generate more work for yourself, if you've got a good process. Testing will have to file a bug report, you'll have to find the bug a later date than review time (which will take longer to track down as it's not so fresh in your mind), and you'll have to give it back to test so they can verify it's fixed. Much better to catch it before it leaves development.

Whilst I could trust you to write competent code, I would never trust you, or any other developer, including myself, to write 100% bug free code all the time. It does _not_ happen.

Your code is not you. Get over it.
Ritchie Swann Send private email
Friday, March 30, 2007
 
 
>> look, you either trust me or you don't to be competent enough to write code. <<

It's clear that you don't trust "them" to review your code competently. So why should they trust you to write your code competently?

You are not special. You are not a beautiful or unique snowflake. You write the same crap code as the rest of us.
Mark Pearce Send private email
Friday, March 30, 2007
 
 
Almost all code that goes into OpenBSD's CVS is reviewed by other commiters. I can't be arsed to find a web-accessible archive of the checkins@ list.

Saturday, March 31, 2007
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz