Fog Creek Software
Discussion Board




Code reviews and formal processes

I work in a company doing military contracts - I'm working on the command system for a rather large ship.  Our company is known for excessive use of formal processes to control the software's development (even for a military company). 
We use PVCS for this - for those who haven't used it, it's a fairly typical source control program, allowing you to delegate code to people, raise change request and observation reports, and force each piece of code through a review lifecycle.  (It's also a horrible piece of software which seems to actively try to slow you down and make things unpleasant, but that's another story...)

We write use cases, then design analysis (using Rose to put together some high level stuff on how the component will work), then detailed design (using Rose to spec out each individual message in the implementation).

Our code reviews generally consist of 3-4 people, including the code author and the relevant architect, doing a code walkthrough, and then there's this horrible form to fill out with details on all the necessary changes that have been spotted.


Anyway...the point to all this was that I'm curious how our system compares to others.  What do you do, and what processes do you go through to get there?

Jon
Friday, August 22, 2003

No code reviews at all, just hack and hack, until we're done.  And fix thousands of bugs all at the end.

I'm not kidding... I work on a console game.

There's got to be a balance somewhere.

Roose
Friday, August 22, 2003

Yep .. sounds like what we do. New feature after new feature gets tossed in the mix whenever management come up with some new idea. Hack, hack, fix bugs, reach release date and have everyone panick as we continue to fix bugs right up until the day the master CD is burnt (inevitably late).

Fun stuff :)

jedidjab79
Friday, August 22, 2003

Got to say all the projects I've worked on (20 years) have been more towards the hack, hack, fix, hope method than the very formal method described in the original post.

Some have been better than others, ranging from every module specified and code reviewed, with it's own test plan, right up (or down) to a three year project for a $1B company where no spec was done and everything was done on the fly.

I have to be honest here. Given the choice of the two extremes, I'd elect for the hack, hack method every time. I know its stressful and wastes time, but at least I can be creative and use my initiative.

With all due respect, the method described in the original post seems to completely stiffle creativity and their coders are really just part of a production line. I wouldn't get much job satisfaction from that.

I have done military work too (projects for the Royal Navy/MoD on a nuclear submarine), but they never required or expected anything but the end result. They didn't care how we got there, as long as it worked.

We even had the blueprints for the nuclear sub in our (unprotected) filing cabinet and I didn't have to sign the OSA. Maybe things are different now, that was a few years ago.

Steve Jones (UK)
Friday, August 22, 2003

"With all due respect, the method described in the original post seems to completely stiffle creativity and their coders are really just part of a production line. "

No respect needed.  I hate it too  :)

Jon
Friday, August 22, 2003

That's fine, we do very the same

Evgeny Gesin /Javadesk/
Friday, August 22, 2003

We lean toward the middle - mostly toward:

"What Works in Software Development: Or, how to be lazy without really trying"

http://magnonel.guild.net/~schwern/talks/What_Works_In_Software_Development/full_slides/slide001.html

If you want source code control that requires code review but doesn't stink, I've heard great things about Aegis.  Oh, and I think it's free. :-)

On any project of signifigance, I'll email the source code to a peer and ask for comments.  On large projects, we may grab a conference room and 3-4 people to hash out the more dangerous parts.

The DoD probably requires you guys to be CMM level 3.  If I were to chase CMM, I'd do it though extreme programming, not UML and RUP.  But that's just my $0.02.


http://use.perl.org/~heusserm/journal/

Matt H.
Friday, August 22, 2003

command and control systems for large warships seems poles apart from consoles.  Or does it?  Does Windows For Warships have DirectX?

i like i
Friday, August 22, 2003

There's a formal description of the hack, hack, hack method here:

  http://www.laputan.org/mud

;)

"I have to be honest here. Given the choice of the two extremes, I'd elect for the hack, hack method every time."

Bravo! I always felt that way too, despite knowing that it was "the wrong answer", and I "should" feel the other way.

I think agile development is a way out; you still get to hack, and you skip the ponderous, excruciatingly tedious design phase, but you can clean up the code as you go (ie, whenever it bothers you), and unit tests let you see that you haven't broken anything.

Indeed, given that most programmers I have encountered, even professional ones, are of the hack, hack, hack school, I'd be tempted to architect by simply ordering first and foremost that all functionality have unit tests. Then the specific implementations don't matter too much.

Peter Breton
Friday, August 22, 2003

The better your programmers are, the less need there is for formal processes.  Refer to Joel's article on the Master chef vs. the McDonald's cook. The Master chef doesn't need any detailed recipe and can improvise on the fly, often throwing ingredients on without using any measuring tools, but produces great results. But the McDonald's cook must follow a specific set of detailed instructions, and be provided with a specific breed of meat and potatoes and lettuce in order to obtain an minimally acceptable result.

That's not to say that processes are bad by any means.  Code review is very valuable even when you are dealing with genius programmers. Just about every good open source project does code reviews, even though they almost never have any formal process around writing detailed specifications and test plans.

T. Norman
Friday, August 22, 2003

The first sentence of the second paragraph should have started "That's not to say *all* processes...".

T. Norman
Friday, August 22, 2003

I used to work at a hack, hack, hack place, and it was a complete disaster.

There was the time one of out guys did a program that was supposed to work like "Norton Disk Doctor" (anoyone remember that program?) to find broken files on our media and stitch them back together.  He shipped it out to a customer, who found that it screwed up their media beyond repair.

There was a time I was asked by my manager to find out if a particular feature had ever been implemented (without hooks to the outside interface)--the programmer who was supposed to do it was long gone, and nobody knew.  The only way to find out was to literally walk through the source code and look for a function that would do what the feature was supposed to do.

We shipped bug fixes that weren't.  We once shipped a system that literallly wouldn't boot, and I had to travel to Australia to fix it.  (It wasn't too much later that I quit)

Some amount of this happens no matter what--every company does a bad patch now & then, but this was just a whole other league; things happened on a daily basis that a small amount of process would've just caught.

Now, I'm not fan of overwhelming process (been in that sort of place too), but nobody'll ever convince me that working with no process is great.  (Let me qualify that a bit--if you're a two-person shop, it's probably a different story; we had 40 people)

Gav

Gav
Friday, August 22, 2003

I used to work for a big company.  We had a middle of the road review process.  When the block of code a person was assigned was done a few people from the team would get together and read through it.  We had one form was basically a record of who attended the meeting. 

Most people hated it.  Some people were arrogant enough to think their code was perfect.  Some people worked in small team anyway so had the extreme programming type of continuous review.  Some people were just shy in groups. 

I can say it helped me on occasion, the reviews of my code picked out some serious flaws, but it didn't stop me from breaking the build for four days once, making 600 people scramble until we figured out the problem. 

D
Friday, August 22, 2003

We do mostly government contracts, though we're ISO 9001 and not CMM. For developers we just have a design review (usually done by email), but no coding review, so you just list or diagram the new classes and modules, the main (but not all) methods it'll have and how they interact, and changes to existing modules, then distribute it to the review committee members and they email back with any comments.  Then you update the document with any changes, if you agree with the suggestions (usually they're helpful). But all the little details and methods and things you didn't think about until later you're free to just put in as needed, then update again at the end when you're done.

Then we have nightly builds and a couple people doing regression testing of anything that was checked in the previous day, so we find out really quick if somebody broke something else.

Also we have coding "style suggestions" but they're not mandatory, so everybody's a little different but it's never been a problem.

Rick
Friday, August 22, 2003

Working on a Navy contract, your experiences are not that out of line.  That your company is "extra" intent on reviews, is probably in hopes of improving their CMM number and making them eligible for even better contracts.

For those "hack, hack, hack" people, that is just not permitted on most government and nearly any military contract.  This includes T.Norman's comment "The better your programmers are, the less need there is for formal processes.  "

I have yet to see any impericle evidence that is true.  Everyone - ABSOLUTELY EVERYONE -- makes mistakes.  Whether they are affordable, depends on your project. 
http://www.byte.com/art/9512/sec6/art1.htm
http://www.fastcompany.com/online/06/writestuff.html

But this is 2003?  Surely we have gotten better! Hardly.  If this week proved anything it is we still have the same issues, but now on a global scale.  For example, the current virus going around the web, was said to have been reported to Microsoft in the original version of MSIE 12 years ago.  I am sorry, but the MS group does not strike me as "dumb folk" so T. Norman's quote does not apply.
http://story.news.yahoo.com/news?tmpl=story&cid=620&ncid=620&e=1&u=/nf/20030821/bs_nf/22135
and a personal favorite: http://catless.ncl.ac.uk/Risks

However, depth of reviews are also a factor of what you are building.  In the second article above, it talks about the Space Shuttle and makes the comment: "the on-board shuttle group produces grown-up software, and the way they do it is by being grown-ups. It may not be sexy, it may not be a coding ego-trip -- but it is the future of software. When you're ready to take the next step -- when you have to write perfect software instead of software that's just good enough -- then it's time to grow up."

A little harsh, and dramatic.  The fact is that most of us are not building software where a bug may drop a $5 billion machine, loaded with rocket fuel onto a heavily populated area.    Command systems on a large ship are something that all sailors, would appreciate being as near bug free as possible.  Game consoles?  I don't know.

However, the process does not help, then it is a waste of time and that is worse, as it takes people away from doing the correct behavior to support a wasted one.

BigRoy
Friday, August 22, 2003


I know a lot of people dislike the very idea of pair programming but it does provide an automatic code review.

The benefit to quality is obvious, but we actually have found that implementation goes faster as well. You don't waste a lot of time trying to find "stupid programmer tricks", plus you are far less likely to have to revisit paired code in the future to fix bugs. Overall time to implement a feature drops.

If you can get over the normal reaction to the idea of coding with a partner, it can be very worthwhile.

anon
Friday, August 22, 2003

I never said that formal processes are ever unnecessary altogether with good programmers. I said that the better they are, the less necessary those processes become.  Surely you must agree that the less competent the developers are, the more important it becomes to have formal processes.

T. Norman
Friday, August 22, 2003

I still think that overdesign is a false economy - trying to anticipate that which cannot be anticipated.

Reading through this thread, I find myself reaffirmed that XP (with or sans pairing) is the way to go - maximum use of code maestros, design only to the level required for coordination, and rely on code reviews heavily.

It also seems like a lot of stories as to why XP/guerilla programming is bad rely on anecdotes about people who simply aren't good programmers. Well of course - referring back to the Joel article, you can't use "We had a McDonald's chef, and he couldn't even make a decent steak tartare on his own" to insist that nobody should be allowed to be a chef.

Philo

Philo
Friday, August 22, 2003

I work for a $5B company and it is mostly hack hack hack fight fire fight fire fight fire. I guess we are not very good coders.  I add version control ,bug tracking and informal code reviews. I hate the code reviews but it keeps me honest. Knowing that I have to explain to someone why I did it a certain way has forced me to do it the "right way" instead of the fast way many times.

The upsides are:
1- the weaker coders are seeing how the better developers do things.
2- There is some level of pain for being lazy. We beat you up pretty good verbally if your code is not maintainable.
3-We all have a sense of the thought process that went into building the code. If we have to modify  a module while the person who wrote it is out on vacation we are that farther ahead of the game.

A neat side affect is that we have not had a 2nd or 3rd shift support call in 10 months and our first shift calls are few and far between.

Note that I get very reasonable deadlines. My sister department has kept things the old way and they are constantly fighting fires. So much so I tease them that they don't fight fires they carry the eternal flame.

Lou
Friday, August 22, 2003

T.  I do agree with you.  The problem is the list of "less competent developers" never seems to include the name of the list maker. 

Many people take code reviews too personally.  "You" are a bad developer.  "You" screwed up.  "You, You, You"

It's code, not art work.  In the world of painting we paint a house, not the Mona Lisa.  I don't have to agree that two shades darker would be better, but I may learn something, or teach you something, if I listen. 

Where I have seen this fall apart are senior developers who dictate changes, but cannot explain why.  The infamous "we never put constructors in... "  "Why not? "  "We just don't.  It's bad code."      If in a code review you cannot have the reason explained to you, or you cannot explain why you are doing something, the discussion is worth having.  Only make a change is someone can convince the group it is worth changing. 

And for sanity, be consistent.  If you are going to following a coding standard, EVERYONE follows it.  Newbie to Senior Architect.  Don't get into the "Bill never does, just leave it alone"  While everyone but Bill has to change their code.

BigRoy
Friday, August 22, 2003

I guess I'll add the point that if you have a massive code review process, and mediocre to sub-mediocre coding talent... absolutely _nothing_ gets done.

I had the misforture (cross misfortune and torture), to work on a U.S. military contract for 6 months once. Another programmer and I were able to fix small bugs, but the system as a whole had become such a mess, despite a cautious, rigorous process, that major improvements were nearly impossible.

I ended up leaving my contract early. It was a violation of my personal morals to be a part of such a taxpayer funded boondoggle. Here's hoping I never have to work another government project again....

Rob VH
Friday, August 22, 2003

"Code reviews and formal processes"?! We don't need no stinkin' code reviews and formal processes!

Don Ho
Friday, August 22, 2003

Having sat through my first code review at a client's office yesterday, I can generally say that it appears to be a grand waste of time to review code that you're outsourcing to a consulting company.  They admit they don't always follow their standards, and they nitpick about everything including indenting (to the point where they specify using tab instead of spaces.)  It wasn't even my code being reviewed and if it wasn't for the fact that I used every ounce of effort into staying awake (3+ hour review), I'm not sure I could have made it through that meeting without blurting out "Don't you have anything better to do?  If not, why are you paying us $100+/hr to do this when you could do it yourselves?"

Anon4Now
Friday, August 22, 2003

Its very common for critical systems - that being a system which fails may endanger the lives of people - to have "high ceremony" processes (I'm taking a phrase from Alistair Cockburn's "Agile Developmet").

It makes sense.  You don't want the BSOD in your screen when you're manning the patriot battery (or whatever).

So features and speed are neglected in favor of precision.  Military contacts are based on heavy top-down specification, so reviews make sense: there is no feture creep, so its easy (ha) to review code against a systems specification for correctness and fault handling. 

Contrast this to consumer software, and fault handling is handled by the end user, and you can deal with the biggest complaints in the next release.

7 parts per million
Friday, August 22, 2003


A lot of artifact heavy formal processes tend to slow you down considerably. How many here work on releases that are more than one year in length?

Again, we use an agile process (XP) and typically release once every six months.

So, to look at it one way, we can implement a feature, fuck it up totally, fix it, and release again before some, less agile, projects get their first try out the door. That's worse case. We actually have better quality now than before we went agile. And we can go even faster, if we wanted to. This is especially valuable if you're working on a new product.

anon
Friday, August 22, 2003


"It makes sense.  You don't want the BSOD in your screen when you're manning the patriot battery (or whatever)."

Patriot's are probably not the best example, since they don't hit what their aimed at anyways. :)

(There's an old story concerning the NVA during the Vietnam war. According to legend they sent a message to their Soviet suppliers: "Stop sending surface to air missiles. Send surface to aircraft missiles").

Anyway, I understand the need for precision, but that's not necessarily a reason you can't go agile. How about a scenario where you developed a simulator test bed for the weapon system first, then iteratively delivered features? That way the military could actually provide feedback during the development instead of at the end where it's frequently too late.

Actually, what typically forces military software into waterfall is the hardware component. The software usually has to wait for the hardware to be designed and built.

anon
Friday, August 22, 2003

"How about a scenario where you developed a simulator test bed for the weapon system first, then iteratively delivered features? That way the military could actually provide feedback during the development instead of at the end where it's frequently too late."

Show of hands - your task is to develop a touchscreen interface for a Patriot battery. If it wasn't in the spec, how many people would think of testing the interface wearing a gas mask and chemical gloves?

Up-front detailed design is trying to practice clairvoyance. Iterative design is being realistic.

Philo

Philo
Friday, August 22, 2003

"Having sat through my first code review at a client's office yesterday, I can generally say that it appears to be a grand waste of time to review code that you're outsourcing to a consulting company.  They admit they don't always follow their standards, and they nitpick about everything including indenting (to the point where they specify using tab instead of spaces.) "

That's just a crap code review.  In a good code review, at most, someone would mention that you didn't follow the standards.  In ones I've been at, it's standard to say up front that formatting/spelling/grammar issues should be sent separately via email.

I'm also not so convinced that good programmers need less process--I just think that the "process" part should take proportionately less of their time.  What I mean is, a good programmer doesn't need to do fewer code reviews-- I've seen plenty of good programmers get caught by stupid mistakes--but their code reviews will probably be pretty fast because for any particular review, there will probably be fewer issues, and so it will be a short review.

Gav

Gav
Friday, August 22, 2003

Do code reviews necessarily have to be meetings? "The Pragmatic Programmer" suggests marking code to be reviewed with a comment indicating such and likewise when the review was done. You might be able to incorporate reporting on what code hadn't been reviewed into your build process as well.

The Masked Coder
Friday, August 22, 2003

Gav - check out what I said. *I* believe good coders need less UP FRONT process, but benefit from code reviews and iterative design (constant customer feedback).

I think trying to lump both up-front and in-process together as "process" gets into baby/bathwater territory, just as XP in general suffers because of one facet (pairing).

Masked Coder - I think some (not all) code reviews *must* be done as a meeting, to get discussions going about methodology. If you're arguing about whitespace, your project is doomed - you're cargo-culting code reviews. Code reviews should be about methodology, algorithms, boundary checking, assumptions checking, etc. Ditto Gav on this issue. (actually it seems to me you have two choices - either cite the spec or shut up)

Philo

Philo
Friday, August 22, 2003

I've seen the whitespace-at-code-reviews too, at a government contract shop. They also had a mandated review process that was totally useless.

If your language and build process supports it, get an automatic code indenter, and argue about something important.

I've also been tempted to avoid all the my-code-vs-your-code ego trips that code reviews engender(*)  by writing Kent Beck's Rules of Simple Design on the whiteboard. Anything that contradicts the rules gets changed. If it's a toss up, then it can stay.

* At the shop I mentioned, you could "punish" people by giving their code a bad review, and "reward" them with a good review.

Peter Breton
Friday, August 22, 2003

>"I'm also not so convinced that good programmers need less process--I just think that the "process" part should take proportionately less of their time.  What I mean is, a good programmer doesn't need to do fewer code reviews-- I've seen plenty of good programmers get caught by stupid mistakes--but their code reviews will probably be pretty fast because for any particular review, there will probably be fewer issues, and so it will be a short review."

Code Review != formal processes.  Code reviews are just one small part of an overall process framework.  By formal processes I mean doing things similar to the CMM way, where every litte thing you do or change must be documented and approved and documents are written about how those are documented, and meta-documents on top of those, etc.

Even though I think every project should do code reviews, they are still not 100% necessary.  I have developed successful systems where no code reviews were done, and I'm sure many of us here have also done so.  Of course, if you don't do code reviews you lower your chances of having a robust and maintainable system, but it's still not *absolutely* necessary.  The better your programmers are, the lower your risk will be if you don't do code reviews (but you'd still be taking a significant risk).

T. Norman
Friday, August 22, 2003

So many of these things, like code reviews and pair programming, are essentially aimed at, and presume, poor programmers.

I wonder if there's something self-perpetuating going on. Companies with processes that presume their programmers are dumb end up with dumb people. Those dumb people don't have a clue anyway, and go along with whatever they're told.

abc
Sunday, August 24, 2003

Certain activities like code reviews only assume that programmers are not perfect, not that they are dumb.

The full-blown CMM stuff is what assumes you have a set of dumb programmers.  Which is a reasonable assumption given that the CMM was developed with the large DoD projects in mind which would have over 100 programmers.  On practically any project of that size, a nontrivial percentage of the programmers will be dumb.

T. Norman
Monday, August 25, 2003

I couldn’t read all of this for feeling sick. So if there are any posts that counter the idiot mentality I am sorry. I was looking for inspiration from the kind of idiots that I have to work with on a daily basis and I came across this bunch of c***ts. Its because of people like you that I have to scrabble through millions of lines of cr*p to try and figure out what the bug is about. You need to write code for the people who have to maintain it and not for yourself satisfaction.
To**ers the lot of you!

sal
Thursday, July 08, 2004

*  Recent Topics

*  Fog Creek Home