Code Reviews - The First to Go Every Time?

Dan...I read this post of yours some time ago, but saw it on the recently viewed list and now feel led to comment. Code reviews *will* happen...it's just a matter of when. It can be before production (as it should be, during development when there is still time to do something about the crappy code), or post production when you're sifting through the convoluted haystack to find the source of a problem.

You already mentioned one big reason code reviews are tossed: everyone is busy and the schedules are tight. Here's another: The reviewer learns after a while that his/her very good suggestions are ignored! In fact, I was helping someone find the source of a problem yesterday and saw exactly that: huge blocks of commented code, variable and recordset declarations that were never used, code clearly borrowed from an unrelated system for example purposes and not used...with variable names that had nothing to do with the current system: all stuff I had pointed out months prior. Should I have to tell someone to wipe and then check for trails later to see if they did?

Of course this isn't always the case. I've seen plenty of staff (the good ones who are really equipped to be developers) use code reviews as a mechanism for picking up new techniques and tricks they can use in their next system. In this case, a code review isn't an exercise in finding all the things wrong that the person ought to have been able to find themselves if they knew what they were doing. The code review is a mechanism for making the system more supportable by providing others with some foundation. It also encourages consistency.

Another plug for code reviews: As developers, we often get tunnel vision when we're embroiled in a project and can really benefit from a second opinion...a sounding board.

No, it's not just you, Dan. I'm at an earlier stage in the process, trying to establish review. It's hard to do, for several reasons.

One thing I think we have to register is that there are cultural preconditions before reviews are useful. I once worked with someone who used to work at ICL, a large and now defunct UK computer company. I called a review for some of my code. He said he would come on one condition: that I promise reviews would not become mandatory. At ICL they were compulsory, and were, he said, a waste of time. This is all too easy to imagine.

When people resist reviews, it's because they don't feel they will be useful. This may be because they don't know the benefits. But they may also be right. I agree with everything you said about the potential benefits, but I think it needs a very high level of maturity.

I plain don't think that this level can be acheived bottom-up. So I don't think any coder or team leader should feel bad that they cannot sustain code review. It needs support from the top, and a thoroughgoing quality culture.

Thanks for adding your thoughts, Chris. I agree, in particular, that sustaining reviews and other quality-oriented processes for which it is difficult to perceive the benefit requires a committment from the top. A quality culture, as you put it, probably cannot happen without support from top either. Along these lines, I'm fond of this essay by Joel Spolsky called "My First BillG Review." After telling a great story about Bill Gates reviewing Joel's spec for an early version of what later became VBA, he concludes with some thoughts about the benefits of having a programmer, someone who understands the technical issues and the software culture, running the show.

Thanks again,
Dan

BillG wasn't a great programmer, but had a complete understanding of the way in which you need to account for everything, including the fact that 1900 had (and 2100 will have, assuming we make it) no Sadie Hawkins day.

But how many bond maturity programs for bonds bought in the 19th century (which still exist) know this?

People have a sense of their lack of worth which was beaten out of them in the early Microsoft culture by diminishing the old sense to a negative value and then building it back up to a larger positive value.

Poor parents in America tell their kids to downsize their expectations, and the father of one rich girl I knew in Seattle regularly called her "dummy". Dave Cutler, the developer of Windows NT, had had a hard childhood and had struggled out of it.

These predamaged souls came to Microsoft for one brief shining moment in the 1980s in which they were called upon to be like Leonard Cohen foresaken almost human.

I'm serious. Microsoft in the 1980s was a subtext of the 1960s in which people said for one brief shining moment that we deserve to do something cool and useful. My observation was that the Microserfs were not as academically qualified as people going to Apple at the same time: state schools as opposed to MIT, in somewhat the same way as, earlier, Tom Watson of IBM recruited salesmen and engineers, who were upon being recruited headed for the American night, ordered them to shape up, and rewarded them for shaping up.

Yet somehow...Douglas Coupland documents their road BACK to the default condition of enserfdom while nobunny writes of the outward bound journey, because it wasn't as sexy as Apple.

BillG was positioned to do this man to man but already, time has moved on.

Today, if even a MANAGER walked into a conference room with a fully marked up document and started firing questions, he'd be met by vacant stares if high enough in the hierarchy or another manager...who takes the position that his concerns are somehow kiss-of-death "academic", that we just need to "get it out the door", ya, de ha ha...would mock the markup guy as a dweeb.

Even if the manager is like Bill a majority owner, he would STILL be subject to exogenous pressures in such a way that resistance would be able to contact board members or venture people and undercut the manager/owner.

The irony is that when BillG was creating something ex nihilo, I allowed some bimbo of a consulting firm executive, in 1979, to tell me that all useful software had been written, for mainframes, and that the software powering embedded devices would use IBM mainframes and their compilers to create binary files for download, period.

I should have told said bimbo to fart in a bottle and paint it, and, a year later, I did. Not only that, on my last day with the firm, knowing she hated smoking, I bought several "nize beeg fat Habana ceegars" and distributed them to the mailroom crew.

"...men will still say, THIS was their finest hour".

[Donna she really was a bimbo she really was, and I am a real feminist guy. Well sort of.]

Unlike Bill Moor I had no habit of surrealist pranks (some of his pranks in Chicago and Berwyn were classic). I had two small children and was deathly afraid of unemployability from 1981 on. Still, man's gotta do what a man's gotta do.

Managers love it when programmers have lousy tools and struggle with buggy compilers. If management in some embedded or similar context makes you use a lousy language or compiler, write a new one.

I too am a big fan of code reviews. I've found them especially useful in guiding young or mediocre developers toward better practices. Or even toward guiding intermediate or advanced developers toward even better practices. I myself have recently learned a lot by having some of my own code reviewed by a developer who works in the IT shop at our client.

That said, I typically look at code review as a once in a while thing--something that's it's good to do from time to time, but not something that needs to be done on every piece of code written. Am I alone in this?

Our client actually has a policy that every single code change that a developer makes must always be code reviewed by another developer. I believe that this process is a waste of time, personally. The occasional code review will reveal a bug or a design flaw. Constant code reviews lead to brain death or will be cursory at best. They are certainly very time consuming and would eat up a huge chunk of our schedule if we actually did them diligently (which now they are putting pressure on us to do, even as the already overbooked schedule really starts heating up).