Code Reviews

Code Reviews should be done on a continuous basis — a good VP of Engineering will continuously keep an eye on the code that is written.

• 3rd party inclusions of libraries/code (Liability, viral GPL (assuming you're writing closed source code), export license issues)
• Poorly performing code
• Coding style issues, causing problems in maintaining code
• Policy/standard issues
• Portability problems (these seem to be one of the most prevalent issues)

Instead of scheduling tedious "torture sessions" — consider just continuously monitoring the source code control checkins, and commenting in innocent "bite size" emails to the developers.
Avoid saying "this is wrong" without offering at least a suggestion as to what might be "right."

For purposes of the bulk of this article, I will refer to C/C++, though I hope to add specific chapters about Java, PHP, Perl, Python and SQL eventually.

I think of "continuous code review" in the following terms (listed bottom-up, work-in-progress):

• C/C++ coding standard / style
  • Comparisons; constants first to catch "==" vs "=" errors, i.e. "if (a =1)" type errors
  • Data structure alignment
    • Often accessed element first (no offset needed in addressing the element, first element is fastest)
    • Odd alignment, big ones first, or use padding (unless prohibitive for size)
    • In packed structures, remember to pad manually, and group items of similar sizes together (BYTES, WORDS, LONGS)
  • Data types
    • Is "int" used appropriately (16 vs 32 vs 64 bits) int is of NOT a particular size, depending on platforms you may end up porting, int can be as small as 16 bits, and as large as 64... is it what you expect it to be?
    • floats/doubles... is there a real reason to use them?
      • Floating point is SLOW (compared to integer math)
      • Floating point is INACCURATE
      • SOAP BOX: if you need to actually read why floating point should be used judiciously... you should give up your keyboard...
  • C++ Exceptions... good and bad
    • I had some interesting conversations on this recently; "surely gcc now implements exceptions efficiently" — nothing like writing a quick sample program, benchmarking the function calls, and once you can't believe your eyes, checking it from the assembly output...
    • Even if you never throw an exception, the mere fact of enabling exceptions is going to wreck havoc on the function call setup
    • This issue won't affect you, if you're writing an application, or something that is not time critical — but for a server, or a real-time system... think twice
• C/C++ components, system calls vs. libraries, constructs/containers
  • Linked list, BTree, basic constructs... I'm sure it's fun to reminisce the college days, and build your own... but not on company time! Even if you're superfast at coding it, remember the time it takes to debug it.
    • There's STL (which really has matured a lot in the last five years, after a rocky start), and it produces remarkably efficient code
    • If you're totally anti-STL, there are other libraries as well
  • System calls, some of my least favorite
    • A recent experience; getservbyname() ... benchmark this one... it is slower than anything I know, for a simple operation — if you do ONE getservbyname() a day, it's ok — otherwise build your own
    • gettimeofday() — hooray! You can get time, all the way into MICROSECONDS! Joy! — except that the time is not MONOTONIC, i.e. time jumps at arbitrary intervals, and can even move backwards... good only for bragging to ignorant audiences. Lesson learned here is that a programmer should really know the hardware he/she is programming... if there is NOT a microsecond clock in the system, then a microsecond time stamp is an approximation, not a fact
  • Leaking things
    • Memory; most common, I would imagine... to always remember to free what you allocated — simplest is to not allocate (malloc/strdup) when you don't need to, instead allocate fixed size from stack, if you can estimate
      • Counterpoint; if you use fixed size buffers, remember to use the N-functions, strncpy instead of strcpy, etc.
    • File handles; far less frequent, but all the more devastating
    • To avoid leaking things, consider programming in Object Oriented style, with proper constructors/destructors... or if you're married to C, this is the one time where "goto's" are ok, to have a common cleanup-and-exit point
    • SQL prepared statements, cursors... this is even more devious — you don't even see the leak in your app — you're leaking resources on the database back-end, which will eventually either bloat or crash