The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

Why people don't do code reviews?

I need to make a list of reasons why people don't do or don't effectivly do code reviews. I have come up with the following:

1. Too much pressure to release.
2. Expertise of reviewers not good enough.
 
What are the other flaws in code review process?

Thanks,
Me
Tuesday, February 27, 2007
 
 
3. Lazy.
4. Tend to dissolve into style argument rather then code semantics.
5. Ego.
Dan Fleet Send private email
Tuesday, February 27, 2007
 
 
The reasons I have been given in the past are mainly time related. Problem is that not spending that time reviewing the code can cost allot more time later on if there was an issue.

Another reason I find is that people are not eager to have their code reviewed perhaps because they feel attacked - this is probably from a bad code review experience (e.g. laughing is bad!)

I have found the occasional pair programming sessions a very effective method of reviewing code - more hands on etc.
Paul Send private email
Tuesday, February 27, 2007
 
 
I don't think people realize code reviews work and they can be done quickly and efficiently. Our typical experience of code reviews is quite painful. This page may help counter this idea: http://www.possibility.com/epowiki/?page=CodeReviews
t
Tuesday, February 27, 2007
 
 
IMHO here is why code reviews are a waste of time.

Take this piece of Lego brick.

Tell me if it is going fit snugly into some vaguely defined location as part of this incomplete Lego model.

You can try, but for anything but the simplest Lego model you will fail.
Jussi Jumppanen
Wednesday, February 28, 2007
 
 
Because reading code is hard.  It's much easier to pick apart naming conventions, indentation and brace placement, and the number of comments per function.

Use an automated tool to validate these things before the review, and you'll get a lot of touchy-feely responses like,"this method seems too long," or "you return from more than one place in this function."

You rarely get,"Why are you using a linked list for this?  The sorting module will crap on that when you insert it into the database," or "This won't scale past 10,000 transactions because you have an implicit critical section here."

Years ago, I came to the conclusion that the biggest benefit of code reviews was not to catch problems in code but to communicate your code to other people on the team before they had to go in and dig out your bugs.
Art Send private email
Wednesday, February 28, 2007
 
 
My team was supposed to do a code review and our manager assigned each of us two project to review. After a few reminders, nothing happened. His manager found the wiki page and sent an email saying it looked like a good idea and wanted people to get them done. Its been almost two months and I'm the only one who did *anything*.

I sent a brief list of files to review, kindly reminded people, requested code lists / presentations of the projects I needed to review, and sent feedback. So far the most I've gotten back is "oh yeah, it looked good". Really helpful.

So the reason is because people don't see it as a priority worth their time so they push back. Its not hard to do a moderate review, since you're mostly just looking to improve style, general quality, and spread design ideas. They don't want to put the energy towards it because its easier to get lost in other activities that are more immediately rewarding.

That said, when they work not only does the baseline improve, but everyone learns something new. So its a good idea, just hard to get done.

Wednesday, February 28, 2007
 
 
Introducing code reviews into an organization that doesn't do them can be incredibly difficult, because it requires a culture-change. A great help is a strong-backed configuration manager who will simply refuse code that is not properly reviewed. Buy-in from the project manager is an absolute requirement.

Where I work, code reviews are a part of life. Every code change is reviewed by at least one other developer. Review effort is estimated and scheduled. From my experience, I agree with Art above: The biggest benefit of reviews is that it spreads knowledge about the code, not that you will deliver bug-free code.

Another benefit, albeit slightly more cynical, is that it forces developers not to cut corners. Knowing someone else will look at your code critically is a great motivator to write decent, properly OO code and stick to coding guidelines.
Jeroen Send private email
Wednesday, February 28, 2007
 
 
The problem is the usual methodology is to hand people HUGE printouts. And then head for a room with no computers in it. And then everyone gets out red pens and their egos.

And at the end of it, you have one developer who goes and updates their CV and spends the rest of the day on Jobsite, a bunch of developers who feel they've moved one place forwards in the World's Greatest Developers lineup by dint of having found someone they're superior to and a massive pile of paper with illegible red stuff scrawled all over it which will be largely ignored and then (if lucky) put in the recycle bin.


Even when you find actual flaws as opposed to I-wouldn't-have-done it-that-way-only-an-idiot-woulds, the discussion is still settled by ego fighting.

Some years ago, in the last code review I went to willingly, I pointed out that getenv() can return null pointers and that this might explain why the system segvs on startup if all the setup scripts haven't been run in the right orders. And we might want to check the return values.

The eventual conclusion, by dint of my not being willing to be the one who shouted loudest, is that getenv definitely does not return null pointers for missing environment variables and this does not need checking by reading the manpage.

Sadly the shouting wasn't loud enough to travel back in time to the early 70s and convince Mr Thompson that this ought to be the case, and so the system remained broken.

But I did get an updated CV out of the day.
Katie Lucas
Wednesday, February 28, 2007
 
 
All the entries above make fairly sound points.  Some of them seem a bit jaded though; I guess thats to be expected. 

I think a big part of the problem with code reviews is that they are usually thought of as outside the scope of a given project and aren't conducted within the bounds of any stated goals or objectives.  The code reviews can't be thought of as tedious busy work that waits at the end of a project like a sour desert for whatever poor soul ends up caring enough to participate. 

There have to be stated and well understood benefits to the business or organization for doing a code review.  From a business perspective, well written code isn't usually a requirement, and in my experience, most companies don't even realize that they should be thinking about it.  From an engineering perspective, code reviews always sound like a great idea, but again, code quality isn't turned into an issue in the technical design.  With that in mind, here's my take on the situation.

1.  Find the benefit.
  If it doesn't benefit the business in some way, its likely that no one will truly care about it, developers included.  If you have a solid technical team with solid technical leadership, identifying and explaining the benefits of code reviews and well written code shouldn't be a problem.  It should be clearly expressed in the technical design and brought up when proposing the implementation.  The benefit should also never be expressed or thought of as "well written code".  The benefits should be things like reusability, ease of understanding by other developers, easie of debugging, and code stability.  If thats expressed and deemed unimportant by the buisness, so be it.  They can deal with all the problems casued by janky code as they come up.  At least it was brought up in terms of how it relates to the business and a decision was made.

2.  Code reviews are highly subjective, so establish clear criteria for success. 
As many of the people who commented on this topic already stated, its very easy for code reviews to end up being more about egos and attitudes than effective code.  Establishing clear criteria for what proper code is before implementation begins makes all the difference.  Outside of the simple and obvious stuff, what's defined as effective code is highly subjective and depends on the organization.  It might be as simple as naming standards for functions and variables or be more elaborate and include establishing architectural and inheritance constraints.  Whatever criteria is chosen, it should be simple, easy to understand, and take very little time to thoroughly test for.  If you do that, code reviews become much less complicated.
Martin Beechen Send private email
Wednesday, February 28, 2007
 
 
Code reviews here are like pair programming. We sit side by side and discuss all of the changes, watching it run and so on. We review EVERYTHING before committing to source.

This has the benefit of catching odd problems (often the reviewee spots things) and making sure knowledge gets spread around the team. This is not a place for a lone gunman.
The Harbour Master Send private email
Wednesday, February 28, 2007
 
 
George Jansen Send private email
Wednesday, February 28, 2007
 
 
+1 to Martin Beechen.

We typically do not do code reviews because it's an economic decision; code that's 98% correct is "good enough" for our purposes. Chasing that extra 1.999% can dramatically inflate the cost of the software.

To view it from another perspective, we can hire someone to write lousy code, and have a good experienced developer to correct it, or we can have the good experienced developer write it the first time. I think this mentality is short-sighted, but as is, nobody has the budget to properly train and mentor "junior" programmers. If you attempt to do the right thing, you'll loose much of your competitive advantage.
TheDavid
Wednesday, February 28, 2007
 
 
We do code reviews, but two more strong temptations not to include:

6: They're very dull and hard work
7: They are not always effective.

To actually catch a logic flaw in a complex piece of new code often requires the reviewer to spend an hour or two looking at the code, which may not be worth it. However code reviews area good for things like:

Checking that your code is adequately documented. Programmers hate to add comments, and this is a good way of making sure they do.

Checking that the coder has adequately dealt with unusual or wrong inputs - check for null pointers, inputs out of range etc.

Forcing the coder to actually look at their own code one more time. It's surprising how often this will catch debug prints left in, or something like that.

A review by a senior developer will also sometimes detect that the original coder has gone about things the wrong way.
DJ Clayworth
Wednesday, February 28, 2007
 
 
I'll chime in that code reviews are absolutely essential. In my experience, the developers who whine about code reviews are the one's that need them the most.

One thing that people seem to be missing is that code reviews help catch the times when programmers don't really understand the spec. This is much more important than coding style or finding a few bugs. The code review is the last chance to verify that the programmer actually understood the requirements before the code goes to QA.

I can't tell you how many times I've caught mistakes or missing functionality because programmers didn't clearly understand the spec. And I can't even count how many major and minor bugs my team has caught and squashed just by having simple code reviews.

For what it's worth, our code review process involves sitting down in the office of the programmer and looking at their screen. It takes very little time for small changes and not much time for the big stuff. There is no need for printouts and big committees. Just look at the code and see what you see.
dood mcdoogle
Wednesday, February 28, 2007
 
 
The definite cost of time vs the unknown in terms of how many defects will be found and whether they could have been found some other way.

I would suggest using automated lint tools to check code statically where possible and unit testing for checking dynamic behaviour.

Then do code reviews on the highest risk and most problematic modules first.

It is recommended practice for a developer to single step through their code in a debugger and this can be extended to a code review.

I have found a data projector is faster than using print outs for code reviews. Especially as you can search and bring up related code on the fly.
Colin Sanson Send private email
Wednesday, February 28, 2007
 
 
Because it takes a LONG time.  60 lines of code (one page)takes 5 minutes, if everybody is disciplined.

All it takes is one yahoo who decides he doesn't like the particular algorithm chosen, to make each page take 30 minutes.  Try that, for a typical almost trivial 1500 LOC / 25 page utility -- you've now spent 12.5 HOURS in code review.  If you have 4 people in on the review, that's 50 HOURS of FTE.

And that's just the review.  We haven't factored end that EVERY SINGLE PAGE can have 3 or more "required modifications", that then MUST BE REVIEWED AGAIN!
---------------------------------------------------

The first issue is -- people don't like to be criticized, so they're reluctant to have their stuff reviewed.  The second issue is -- people LOVE to criticize other people's stuff, so the first issue is a very real one.

The third issue is -- people are very bad at being disciplined, looking at somebody else's code, and focussing on UNDERSTANDING it, instead of CRITICIZING it.

And the fourth issue is -- without that discipline, a code review becomes a very unpleasant, very expensive, very time-consuming pissing match.

Solutions that merely say "Well, be DISCIPLINED about it then!" are ignoring something very fundamental about human nature.

It is POSSIBLE to have valuable code reviews.  In my 15 years of trying, I have not seen one.
AllanL5
Wednesday, February 28, 2007
 
 
"If you have 4 people in on the review, that's 50 HOURS of FTE.... It is POSSIBLE to have valuable code reviews.  In my 15 years of trying, I have not seen one. "

That's because you are trying to do a code review by committee. Why do you need 4 people? Other people mention using a projector implying that there is a big conference room involved. What gives?

ONE person sits down with the developer at his/her computer and has them explain the code. Nothing more, nothing less. Once you've done a couple it will only take a couple of minutes for a simple change. The longest code review I've ever had took 30 minutes. And this was for a task that originally took 200 hours to complete. You don't need to review every stinking line. Look at the big picture. That's all. You are trying to make sure that the developer has done the following:

1) Understood and completed the requirements.

2) Followed coding standards.

3) Understands their own code (you'd be surprised at how many don't).

4) Has not made any glaring errors.

There is no reason for political battles or a big committee arguing about the best way to do something. The reason why people don't like code reviews is because they simply don't know how to conduct one.
dood mcdoogle
Wednesday, February 28, 2007
 
 
> To actually catch a logic flaw in a complex piece of new code often requires the reviewer to spend an hour or two looking at the code, which may not be worth it.

As the senior developer I'd review new developers' code; and stop reviewing their code after my reviews stop finding bugs.

> ONE person sits down with the developer at his/her computer and has them explain the code. ... The longest code review I've ever had took 30 minutes. And this was for a task that originally took 200 hours to complete. You don't need to review every stinking line.

If I do a code review I don't do that (I want to verify whether I, and the computer, can understand the code, without the original developer).

* Developer announces "code complete (and tested)" and submits for review
* I review it (in my own time, alone, at my own desk); I may or may not (depending on the developer and the code) review every line.
* I write a list of the problems that I find and want to discuss
* Meet to discuss the problems (if any)

Two categories of comment:
1. Bug or potential bug: must fix;
2. Coding style suggestion: no fix this time, but keep it in mind next time you write some code.

If a fix was required, the second review typically only needs to review the diff (in fact, the initial review was also typically a review of the diff).
Christopher Wells Send private email
Wednesday, February 28, 2007
 
 
For many people it's an ego killer. It's similar to the reasoning behind holding off doing personnel reviews until after the development phase has reached the test phase. Some people haven't reached a maturity level where they can handle criticism of their work. Unfortunately these are the same people who are polishing their CVs instead of reflecting on making themselves better.

Nit picking the coding semantics is a waste of effort and best done by something like FxCop. A good code review should have both the developer and the reviewer looking at the code together. The reviewer can then ask questions for things that don't make sense. Hopefully they gain some insight and ask more pertinent questions that the developer can reflect on. The real value is in being able to explain the logic to another developer who can who can act as a filter. Although, many times in explaining the logic the developer has an inflexion point where they look at the problem from a slightly different view.
Angstrom
Wednesday, February 28, 2007
 
 
Code reviews are very useful. For those at companies where they don't work, it's a management failure. Management needs to recognize and understand the value of reviews and implement policies so that they work. Maybe one of those policies is that they are always voluntary reviews.
Meghraj Reddy
Thursday, March 01, 2007
 
 
I've certainly found code reviews useful. However we always do one-person reviews, and they hardly ever take more than a few minutes for a small change and less than an hour for a big one.

The only time I've done multi-person reviews is when I'm making a change in fragile code very close to release - then I just want as many eyes as possible making sure I haven't forgotten a case and haven't introduced a side-effect.
DJ Clayworth
Thursday, March 01, 2007
 
 
I guess this answers the question -- because there are so many definitions of what a "code review" is.  And so many of those definitions result in very bad situations.  So it is MUCH safer to say "I don't do code reviews" than to be sucked in to some poorly defined mud puddle.

But I'm glad to hear some are having success with "one other individual reviewing the code at his desk" reviews.
AllanL5
Thursday, March 01, 2007
 
 
Yeah, I think the 'one other individual' plan is the best. I've used that sucessfully in the past, doing something like pair programming except that instead of having one programmer watch over the other's sholder, they checkin every few hours (maybe twice a day) and review code.
Dordal Send private email
Thursday, March 01, 2007
 
 
Code reviews are like a jumbo-sized magnifying glass on problems in your software development methodology. Nobody *notices*, for example, how inconsistent all your code is without standardized coding conventions until everybody has to look at it. Then you realize what you've done has this *amateurish* feeling.

It's no wonder the emotions boil over during code reviews, everyone is required to see the naked truth, unfiltered by the usual lens of "hey, it works!"

Code reviews: not for the faint at heart.
Robby Slaughter Send private email
Thursday, March 01, 2007
 
 
> "one other individual reviewing the code at his desk" reviews

It's an instance of the general rule of preparing before a meeting.

If I'm reviewing a finished technical document, for example, then I'll often prefer to:

* Send for review
* Reviewers read it in their own time, and prepare a list of controversies to discuss
* Meet to discuss the listed controversies

I think that's better than ...

* Finish the document
* Schedule a review meeting
* Meet together to read the document for the first time, and to discuss whatever comes to mind

... because:

* You can spend as much time as you want on reviewing it
* The meeting is more focussed (its agenda is the list of prepared controversies)

Note that I said 'finished' document: this kind of "over the wall" review process is probably more appropriate for a final review (of 'finished' code), but not for something that's still being designed and developed (which should more interactive and less formal).
Christopher Wells Send private email
Thursday, March 01, 2007
 
 
I think code reviews can be very valuable in some circumstances, as long as they aren't applied globally to everything. Here's just one small section of SQL code that I reviewed yesterday:

EXEC    dbo.CleanupExceptions
IF    (@@error != 0)
BEGIN
    SET @ErrorText = 'Clean-up failed! ERROR:' + 
            CAST(@@error as varchar(10))
    RAISERROR(@ErrorText, 16, 1)
END

The issues that I pointed out:

* Not checking stored proc return value.
* Not noticing that the error check resets @@error to zero.
* Not doing a rollback if an error occurred - this doesn't always happen unless you're in trigger context.
* Not returning after an error.

One reason code reviews are controversial is because egos are often at stake. So there needs to be some agreed mechanism for dispute prevention, and another for dispute resolution.
Mark Pearce Send private email
Friday, March 02, 2007
 
 
I think many replies bring up a good point about the *issues* that sometimes rise in code reviews, especially with emotionally immature people.

However the question of why *most* people don't do them?  I think most developers would not even think to do code reviews.  It'd literally never occur to them.  The second leading cause of code review death is probably management that doesn't see the value of people sitting in a room staring at computer code.
Crimson Send private email
Friday, March 02, 2007
 
 
Those interested in code reviews might find it worthwhile to read about Mondrian, the code review platform used within Google and created by the author of the Python programming language.  It's not available to the public, but the ideas might be useful to anyone considering implementing a code review system in their workplace.

http://www.niallkennedy.com/blog/archives/2006/11/google-mondrian.html
Dev
Saturday, March 03, 2007
 
 
> I can't tell you how many times I've caught mistakes or missing functionality because programmers didn't clearly understand the spec. And I can't even count how many major and minor bugs my team has caught and squashed just by having simple code reviews.

I can't count them either, as they never come up in code reviews.  QA or the person that requested a feature usually catches those as they are the stakeholder.  Who better to know what the flow of an application or what should happen than the person asking for it?

If a programmer didn't understand the spec it is likely that other programmers didn't either or that there's some problem that's not going to be found in a code review.  This really only comes up when QA looks at it.
BlogReader Send private email
Monday, March 05, 2007
 
 
I work with 20 people who code in C#. I code in C++. They all went to Java schools and don't know C++.

Can they review my code?
Anon
Monday, March 05, 2007
 
 
Of course not - you're much too l33t!
Awestruck
Wednesday, March 07, 2007
 
 
AllanL5:  Solutions that merely say "Well, be DISCIPLINED about it then!" are ignoring something very fundamental about human nature.

Well, civil engineers are human too, yet they seem to be disciplined enough to look over each other's work before a bridge or skyscraper gets built.  Likewise, chemists and biologists are human, yet they seem to be disciplined enough to read other lab members' protocols before adopting new protocols.

When programmers refuse to use good engineering practice, they are not reflecting human nature in general.  They are just acting like prima donnas.
JH Send private email
Thursday, March 08, 2007
 
 
For years I have found short review sessions with at least one other programmer very useful. It takes time for a team to learn to work together and for a mutual respect to develop. When that point is reached code reviews can be plesant learning and team confidence building experiences.

Of course, in some teams there will be primadonnas or simply weak programmers who figured out that their only available career path is to become managers.

A lot depends on company's culture, experience and sincerity of the upper management. It may be hard to get even a good team motivated to do meaningful and regular code reviews if they understand it as nothing else but otherwise disinterested manager writing pluses to himself for the review by his manager.

Saturday, March 24, 2007
 
 

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

Other recent topics Other recent topics
 
Powered by FogBugz