Quite a while back, I was managing a group of C programmers who'd developed some extraordinarily bad habits, not the least of which was no code reviews. In the course of instituting code reviews, I wound up writing a document explaining (to the uninitiated) some of the whys of doing a code review. With these principles in hand, it was much easier for people to understand why we were doing things differently.

Excerpts follow (I've not included items which were specific to that project or clearly overly C-related, although there's still a bit of the latter poking through):

Why Do We Do Code Reviews?

• To find and eliminate bugs before anyone important sees them

  Every bug which is removed during the code/unit test/integration test phases is one less bug which has a chance of finding its way into the finished product and into a customer's workflow. Code reviews allow others to look at a piece of code and to consider if there are logic errors.

• To find ways to prevent bugs or make them automatically apparent

  We need to determine ways to either not write bugs or make them jump up and announce themselves when they occur. Our goal is to do the fun stuff of writing more, better, code, not the dull part of spending our time finding bugs.

• To find and eliminate styles of coding which are bug-prone

  Bugs can be introduced from a variety of sources, including techniques which make it difficult to either track what is happening or to see what is changing. We need to eliminate the stylistic approaches which make this sort of development possible so we increase our chances of not writing buggy code to begin with.

• To encourage and enforce common coding styles

  If we have common coding styles, it makes it easier for any developer to pick up a piece of code and have a chance at figuring out what it does. This is especially important for the 2:00 AM phone call about a system problem.

• To allow others to suggest better methods and coding practices

  There is almost always a better algorithm or approach to any problem. By conducting a code review, other developers can provide input to find out if there are better (simpler, easier) ways to do anything. If so, let's implement them.

• To disseminate methods and coding practices to others for their benefit

  Sometimes we stumble across a truly better way of doing anything. We need to make sure that other developers find out about it so we can spread it through the team and project.

Various Stylistic Thoughts

• The function of code is for our customers; the form is for our successors

  Every program which is written has both a function and a form - the what and how of the code. The customer for a program's function is, indeed, the end-user customer. However, the customer for a program's form is the programmer who must, bleary-eyed, debug a production problem at 2:00 in the morning. Make the form clear to even a novice programmer so even a novice can fix it.

• Compiler warnings are bad

  There are very few warnings which are impossible to make go away. For these warnings, understand them and make a comment about it in the code. Otherwise, create type casts or use parentheses or whatever is necessary to make the warnings go away.

• Don't try to out-optimize the compiler except in technique

  Clear code is more important than hand-tuned code. Most modern compilers can out-optimize hand-tuned assembly language. Spend your time productively - writing new, clear functionality, not trying to out-think the compiler. It is more important to make the purpose of code clear than it is to come up with the absolutely, positively most efficient approach (but make sure the purpose is clear).

• Don't write the same code more than once unless you absolutely have to

  If you find yourself writing the same exact code more than once, this is a clear indication that one of the following is true:

  • You have mis-rolled a loop and should rethink it.
  • YOu have a common routine which should be put in a separate function.

• Make variable use obvious

  Variables should either be completely self-documenting (by grace of their name) or should have a comment explaining their use when they are declared.

I also handed out a copy of Steve McConnell's Code Complete to everyone, along with the comment that I didn't agree with everything he said, but I could explain where and why I didn't.

---

---

• Are you posting in the right place? Check out Where do I post X? to know for sure.
• Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:

  <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>

• Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
• Want more info? How to link or How to display code and escape characters are good places to start.