How The Game Is Played

Why I don't believe in Code Review

Why I don't believe in code review.

Note: A good observation came back from comments. I should make clear that I am talking in this piece about the way I have seen code review implemented-- as a blanket requirement that every line of code must be reviewed. As a point technique I am quite sure it can be quite valuable. I've just never had the privilege of seeing it used that way. -- jk

Anyone who knows me knows that I consider questioning orthodoxies the duty of every thinking member of any organization. In my book the question "why" is never well answered with "that's the way we do things here." It is far too easy to fall into wasteful or even counter-productive habits by accepting such a reason. Often behaviors persist as "standard practice" far after the reasons they were originally put into place have changed. Sometimes, standard practices just arise out of a prejudice or an idea that was never more then half-baked to start with.

Today I want to address a standard Sun practice and why I feel it may in fact be counter-productive. That practice is "code-review". Code review is the rule that at least one other person must look over any code you write for any errors they can spot or things they can improve. It is a form of peer-review and peer-review, under some circumstances, can be very valuable. I personally don't believe software development to be one of those circumstances, at least in the general case.

To understand my perspective you need to understand that most of my programming career has been spent in an industry where efficiency is king-- the game industry. Efficiency of your code is important because well tuned game code provides a better user experience. Efficiency of the coding process is also important because you are developing on a fixed time and budget and the more programming work you can accomplish in that time, the more fully featured and exciting your product will be. The keys to being a successful game programmer are producing working, efficient code as quickly as possible.

Code review is virtually unheard of in the game industry.

Why is this? Well, I believe there are two good arguments why general code review is antithetical to the game programming process. The first is a logical argument and the second is a philosophical one.

In order to logically analyze the value of code review we need to make a clear assessment of its costs and returns. Anyone I have ever talked to who does really thorough code reviews has told me that it takes them more time and energy to review someone else's code then it would take to write that code themselves.

This is not surprising. Coding is a complex process and the competent engineer is juggling and evolving a highly complex model of the code-flow in their head as they code. For someone else to read that code and grok that bigger picture is a major bit of work.

My first 3 jobs in the industry all started by having someone else's code dropped on my desk and being told "Here. We need someone to understand and maintain this." To do this right, I had to spend a lot of time reading and analyzing the code, until I could "get into the original coder's head" and understand what they were thinking when they wrote it. It a lot of work and you never succeed 100%.

But think about that first statement. What it says really is this: Code review more then doubles the man-power cost of writing that piece of code. Thats an awfully big "nut to crack" as we say in the entertainment industry in terms of value code review must return for it to be a net gain.

So what are the values of code review? Lets go down what I understand the claimed values to be and examine them in order.

• Code review improves code quality.

  This is a pretty general glittering-generality, but lets see if we can't make it more specific. The idea here is that each coder has individual tricks and techniques they have developed on the micro-level that improve the reliability and/or performance of their code. By looking over each other's code they share the value of these things.

  Thats all well and good, but what is the real value? I contend that any competent coder above entry level should already know how to write reliable code and that they should be expected to write reliable code. If this is the expected case, is it really worth doubling the cost of code creation just to find the rare case when it is not? I don't tend to think so. Furthermore, I fundamentally believe that it is a waste of human brains to make them do work that a computer can do just as well as fast or faster.

  In the case of code correctness, I think that less effort can be spent more productively by defining the code contracts clearly and then writing complete tests to test code against those contracts. Josh Bloch sums the first part of this up well in his book Effective Java when he says "always design with interfaces first." Engineers that write sloppy code,in my experience, make themselves well known through the results of their coding and corrective measures can then be taken once there is a known problem.

  Similarly, there is an important rule in performance tuning. What doesn't matter, doesn't matter. Anyone who does serious performance coding, and all game programmers do, knows that striving for maximal efficiency across all your code is not just wasteful but often counter productive. The most efficient code is not necessarily the cleanest, clearest or most maintainable. But efficiency in the right places can be critical to over-all program performance. The general rule is called the "90/10" rule. In general 10% of your code does 90% of the work and it is that 10% that needs to be tuned for efficiency. The rest should be designed for readability and maintainability.

  The problem with a code review as a mechanism to attain this proper balance is that it is impossible for a human being to correctly statically analyze the performance characteristics of any significantly complex program. There is simply too much happening on the fly inside of such a program for you to be able to approximate its behavior correctly enough in your head.

  Something I have lectured on time and again in my role as a Java performance guru is that all performance tuning work begins with measurement. That is why a Profiler is your one absolutely critical tools in tuning code performance. Once you have determined what 10% of your code contains your bottlenecks, then and only then can you properly tune it.

  You might be noticing a pattern here. I contend that to create truly high code quality takes active test, measurement and management of your code base. The problem with code review is that it is passive, treating all code as equally important. In the forest of code it is way too easy to thus spend inordinate amounts of time on things that don't really matter and miss things that do. This is not to say that peer opinions aren't valuable, but in my opinion they aught to be confined to code that really matters, and the best way to do that is to allow the engineer to profile and test his or her code and then ask peers for help where it is needed.

• Code review improves code maintainability

  Maintainability is a funny concept in coding. We generally think of it as the ability to upgrade and improve code, but more code is ruined by "maintainence" that pushes it beyond its originally designed in limits then any thing else in computer science. The newsletter of the Society of Computer Professionals for Social responsibility summed it up this way many years ago: "Elevators break down if they aren't maintained. Code breaks down if it is!"

  Nonetheless there are some desired and desirable characteristics of maintainable code. Having been in the code maintainence business my first 3 years in the industry I can say that naming conventions are a Good Thing(tm). Having said that, uniform naming conventions I honestly think are over-rated. In each of the cases I mentioned above I had C code dropped on me written by different people in different settings and thus with different naming conventions. I would say it took me maybe 3 hours in each case to get used to the coder's conventions-- an almost insignificant amount of time in the total scheme of the work.

  More importantly though, as I've already said, I don't believe that you should make people do things that computers can do just as well and faster. Java has standard naming conventions which is great. Why we don't have a lint-like tool to check them though is frankly beyond me. This is the sort of job a compiler or compiler-like program can do over hundreds of thousands of lines of code without breaking a sweat. So let the compiler do it, and don't load humans with the responsibility to review code for it.

  Something that, for similar reasons, I think is even sillier are formatting conventions. The fact of the matter is that readability of formatting conventions is highly subjective. There is no one right answer. The tools exist to reformat code however you like to read it completely on the fly. So rather then trying to push everyone into coding in the same format, just let everyone format the code they want to read however they are most comfortable reading it. One could easily imagine an editor that could show you your preferred "view" of code while maintaining the existing structure in the actual data. All in all, this is another thing computers do better then humans and humans should not be bothered with.

  Adequate commenting can be a big help in later reading of code. However IMO adequate commenting should be the duty and responsibility of any trained engineer the same way correctness is a duty. I am a big believer in empowerment and responsibility of working engineers. I am also a believer in alibi books. Any project should define clear standards for how much of what kind of documentation is required along with any other project standards for work. Any engineer on that project should either live up to that contract or, where they break it, explain the reasons in an alibi-book. This should be the responsibility of the engineer and something on which their performance is reviewed at the appropriate review times.

Thats my summation of the logical costs/benefits as I see them. But I said there was a philosophical component to my belief as well. To understand that, you need to understand how I view coding. Fundamentally, I don't believe software engineers are engineers at all. I believe software engineers are artists. We just call ourselves engineers because artists get paid crap. At the end of the day, I believe that our job has much more in common with writing or painting then calculating the stress forces on a house. I think we give this away in the way we talk about our work. The highest compliment one can give a piece of code is that it is "elegant." Elegance is not a measurable quantity, rather it is an aesthetic judgment. An artistic judgment.

I do not believe that artists do their best work when their brush-strokes are micro-managed and that is what I see code review as doing. Van Gogh's hand likely slipped in a number of places when painting Starry Night. In fact, having studied the painting, I strongly suspect he was using techniques that were not considered "proper brush strokes" at all at the time he painted it. Do you care? Of course not, because the result of his entire work is successful at its goal and is a magnificent success.

Uniformity and the drive to it is the death of art. If you don't believe me, go spend at least 3 hours straight in a Thomas Kincaide gallery. If you can do that without going insane you are either incredibly strong willed or have no artistic soul at all. As I believe the best code is art, I believe a drive to uniform form on the micro-level is also deadly to it. I believe that competent engineers should be given a clearly defined problem (often defined through a software specification and/or a set of Interfaces) and left alone to produce the best solution in their own brush-strokes. Where there are problems, be they engineering sloppiness, critical code inefficiencies, or just plain errors I believe it is the duty of the coding engineer to isolate and deal with them specifically and on an individual basis.

All in all, I honestly think general code review does far more to create and foster religious arguments over style then it actually does to improve either quality or readability of a code-base. I also think it is, at best, a major waste of very valuable engineering talent and, at worst, actually a distraction and a detriment to the creation of exceptional software products.

As usual. Your Mileage May Vary.

Posted at 08:30PM Jan 11, 2006 by gameguy in General  |  Comments[9]