How Code Review Fits Into the Development Process

Code review needs to be understood as a single tool in a toolbox that hopefully contains many tools that give feedback on the quality of the software development process. Which tool is most appropriate in a given situation depends on how much time and money are available for removing defects, and how much each remaining defect costs. Many tools point to defects in development

• code review
• linters, static checkers, and compiler error messages
• test suites
• style checkers

Code review is very good at finding weaknesses in the way individual developers understand their programming language tool. Developers who get busted even once or twice for (for instance) not destroying dynamic data or not initializing data structures quickly internalize these skills.

Human code review is good at finding coding errors that escaped the compiler, but static checkers also find some of these errors. Static checkers have little up-front cost and negligable cost to re-run, but their coverage is limited. By contrast, human code review is expensive each time you do it, but they have as much coverage as you can afford.

As a human process, code review is not well suited to reveal defects like performance issues, that require both a specification of acceptable performance and an accurate measurement of a running system. Human processes are also expensive to use for checking conformance with style guidelines, for which fully automated tools are generally available. This doesn't mean you can't use code review to find these defects. It just means there may be more suitable tools.

Human review of interfaces (for instance C++ header files) is good at finding squishy issues that automated tools aren't any good at finding. There isn't an automated tool yet that can judge the quality of a class or library API. A bad API isn't *wrong* per se. It's just more confused or more difficult to use than it needs to be. Class level APIs aren't captured as user requirements, but evolve out of design and use. Test cases developed early in the class design are also good at detecting API issues. The test writer goes to test a feature, and finds they can't get at the feature in a useful way. The advantage of test cases is that they are inexpensive to re-run if the API implementation changes, while code review is expensive every time you use it.

Pair programming is the ultimate in code review. While one person types code, another reads the code and makes suggestions. It's also the most expensive form of review, costing a man hour of review for each man hour spent coding. I think code review meetings have almost the same coverage, but at a far lower cost.

There are automated tools that seek to make code review efficient. You can get your source control system to hold off checkins until people have signaled approval of the code. I find these tools are less effective than a code review that involves an actual face-to-face meeting. When each developer reviews in isolation, it's too easy to get lazy and perform a prefunctory read of the changed lines before signing off on the review. I used a code review process that required face-to-face meetings among three or more participants. I felt the cost was justified by the quality of the review.

How I Do a Code Review

I led a team to do a design review of every base class in a 40-man-year project; something like 200 C++ class definition .h files. If there was code in the corresponding .cpp file, we sometimes reviewed that too, but we performed this review early in the design phase, so there was often only a header file of 100-200 lines.

We agreed as a team to follow good-meeting rules. Every design review meeting required a minimum of 24 hours notice, and the review materials also had to be complete and distributed 24 hours in advance. If the materials were not ready in time, the meeting was rescheduled. Each participant was required to have reviewed the materials in advance, and made notes. At the beginning of the meeting, the facilitator asked if everybody had done their homework. If there were not at least three people who had come prepared, the meeting was terminated and rescheduled. Meetings were held face-to-face in a conference room, were scheduled for 60 minutes, started on time, and were stopped and rescheduled if they ran over. In this way, we ensured that the client (the person whose class was being reviewed) had prepared, and that a high quality discussion would justify the cost of pulling three or more people away from work for an hour.

A facilitator other than the client was selected for each meeting. The facilitator ensured the meeting stayed on topic, took notes on issues discussed, and circulated the notes to the participants after the meeting. After the meeting, the client edited the notes to indicate what action they'd taken on each issue. The notes were archived with the other project files.

The materials prepared for each meeting were the class's .h file listing, printed with line numbers for easy referencing, the class's CRC card (a concise and structured form of design document), and any diagrams or other materials the client considered relevant.

Since I knew that having your code reviewed could be ego-busting, and because I was a relatively experienced C++ developer, I became the client of the first two or three review meetings. One point of these first meetings was to demonstrate that even an experienced developer could benefit from a design review, and the team did indeed find issues in my lovely code that needed addressing.

When the inevitable snarky comments started coming in that first meeting, I reminded the team, "Today it's my turn. Next week, it'll be you. Lets keep the tone professional, because you want it to be professional when it's your turn in the spotlight." This set the tone for all the meetings to follow. I don't believe we had a single incident of hurt feelings over the whole project.

After the first couple of meetings, it became clear that there were standard questions we would ask about every class. I compiled a list of these questions and archived it as a project document. (They read like Cliff Notes for Meyers' Effective C++). Within a couple of weeks, these issues disappeared from classes entering the review process.

The cost of preparation, and the keenness of team questions pushed every developer to produce high quality artifacts for review. This had a corresponding positive impact on the quality of subsequent code. Questions about destroying allocated objects led the whole team to take up the use of smart pointers. The RAII idiom was demonstrated and became commonplace in the code. In this way, the best developers taught good practice to the rest of the team, who were learning C++ as they wrote the code.

Later on, as the project approached code complete, we began to review the code, using the same meeting process. These code reviews captured many defects that had escaped testing. Our intention was to review only the most critical code, nominated by individual developers. By the end of the project, we had covered about 60% of the code with at least one review.

The software for this project achieved all its design goals and delivered high initial quality. During the development, our company decided to do ISO 9000. This project received the highest marks from the auditors over a period of a couple of years. Subjectively, we all agreed that we delivered significantly higher quality work than previous projects.

One important aspect of the review process was the team-oriented, consensual model we adopted. Developers were not required to remedy every issue raised at reviews. There was no sign-off or other coercive process in place. The team came to respect the review process precicely because it removed defects from their code. It also respected their individual judgement, both as coders and as reviewers.

The cost of the reviews was significant. My own personal experience was that I spent several days after I was "finished" with a class, getting it ready for review, getting the documentation complete, printing listings, etc. On balance, I thought the cost was justified. I would use the same process again.

Things Only Taught by Time

What do you learn over a career of coding, besides 123 different APIs? What's all this value in being an Old Hand? Well...

• When I was interviewing for work, right out of college, there was a job I didn't take because all the developers had foot-thick listings on top of their filing cabinets. I knew my modest brain could never comprehend such a massive amount of code. Ten years later, I delayed learning Windows programming because applications appeared indecipherably complex, with dlls and a configuration database instead of a single executable.
  
  I don't print paper listings any longer, but if I did, mine would be ten feet tall. I've built many Windows apps. It sucks to manage dlls, but not because they're too complex to comprehend.
  
  LESSON: Anything that other programmers are comfortable with is not too complex for you.
  

• I spent 10 years becoming a deep domain expert at my first company out of college. I figured I would become a "lifer", safe from the travails of the job market because of my valuable knowledge. Then during an economic downturn, the company changed strategic direction. They shuttered my whole division, laying me off along with 19 of 21 engineers, 12 of 12 marketing folks, and about 50 factory workers.
  
  It was hard finding a new job because (1) it was the bottom of a recession, (2) my deep domain knowledge was not applicable at any other employer in the city, and (3) I had neglected to learn the latest programming skills because I didn't believe, as a lifer, that I would need them to be up-to-date.
  
  LESSON: Skills and experience are only valuable if they help you find work.
  
  LESSON: Skills and experience that help you find work are valuable.
  

• The two engineers that my one-time lifetime employer retained? One was a very lucky new college hire. The company valued its reputation for firm job offers. The other survivor was a nice lady; very quiet, someone who never asked questions or made requests. She wasn't the smartest engineer. She wasn't the most innovative. But she turned out an utterly reliable so-many-lines of code each month without variation. She became a lifer at that company.
  
  LESSON: what managers value in a software developer is not intelligence, or innovation, or great code. They value reliably low maintenance workers.
  

• My very favorite sister-in-law died of cancer at 39 years of age. When her cancer was diagnosed, she didn't quit her job right away, but she did change her behavior. All of a sudden there were some meetings and some tasks that seemed so unnecessary that they offended her sense of limited remaining time. She told me that a week before her diagnosis, she had wasted a bunch of time in these meetings, and all of a sudden she really wanted that time back. She began focusing exclusively on the parts of her job that added value and that gave her satisfaction. She put off the dumb stuff as long as she could. She discovered that lots of dumb stuff eventually just went away if she put it off, because it was dumb stuff. She didn't lose touch by skipping boring meetings. They were boring because nothing happened in them. In this way she became recognized by her managers as the most productive worker in her office.
  
  I was going to boring meetings where nothing happened too. I vowed to behave as though I had six months to live. I became more productive, because I only did activities that added real value, and that I enjoyed. I came to feel as though I had become bulletproof. I reasoned that if I was ever let go for only doing productive work, the company would be doing me a tremendous favor. Such a company would not last long, and working there until the end would be awful.
  
  LESSON: Only do tasks that add value. You will be the most productive member of your team.

• I became unemployed in the dot.com crash, becaue my dot.com employer collapsed. It was hard to find work because it was the dot.com crash, and nobody was hiring. It wasn't that I had the wrong skills or wanted too much money. There just wasn't anyplace to send a resume. It turned out that I wasn't bulletproof after all. I went from being the most employable guy I knew to being chronically unemployed. I became unemployed again in the Great Recession. Two employers in a row suffered dramatic, thirty per-cent revenue declines, and laid off their whole software team.
  
  LESSON: Bulletproof is not the same as invulnerable. Pride goeth before a fall.
  
  LESSON: Most software development work is project-oriented. Your job is always vulnerable between projects.
  
  LESSON: It is always the bottom of a recession when you get laid off. Nobody will be starting new projects then. It is prudent to have money in the bank to last you a year or so.
  

Looking back on these lessons, they seem very obvious. But each one had to be learned. They were all novel thoughts until after they happened. I'm not sure it's even possible for a rookie to internalize these lessons because the "duh" only comes with experience.

Time to Change Jobs?

It sucks to be laid off. On top of the emotional response to being rejected and discarded, you have to begin a job hunt from a standing start, while simultaneously finding ways to keep food on the table. It's better for both your ego and your wallet if you pick the day of your departure and move directly to a new job.

When is it time to change jobs?

Change jobs ASAP if you think the management is leading the company in the wrong direction, if they are mistreating you or other employees, or if they want you to do something illegal or unethical. There is no future in such a job. Even if you stay, you'll either get fired (because managers can tell when you think they're leading the company to trouble), or the company will fold (because the managers led the company to trouble).

Change jobs if your employer is working you 60 hours every week and they aren't paying you a salary-and-a-half. There are lots of companies where employees work a reasonable number of hours. Most companies that make you work long hours don't pay any better than average. Why give them your life for nothing? Yeah, every job has crunch time when a deadline looms and you gotta put in the hours. I'm not talking about that. I'm talking about companies who think all developers should work 60 hours a week because the founder worked 60 hours a week at Microsoft back before the IPO and got rich on stock options. Stock options are not all created equal. Most expire worthless.

Change jobs if you aren't challenged by what you're doing, and you like to be challenged. HOWEVER, be mindful that you will continue to discover new and better ways to use whatever tools you're using for five years or more. Folks just out of school very often think they know all they will ever need to know by the time they are 25. This is so totally not true, but you won't realize it until you're 35 or 40. Find an old hand you respect and ask them. They will say the same thing. You will just have to trust them until you feel it for yourself.

Don't change jobs if you are really happy where you are. Happy matters a lot more than rich. That's my advice. HOWEVER, don't confuse mere comfort with happiness. It's easy to get comfortable, thinking you know everything in the world about some mature product written in some old language, and that you can set the cruise control and be employed for life. But your company may suddenly drop that mature product, and then where will you be with your out-of-date skills and no relevant industry experience?

Change jobs if what you're doing at this job won't help you get another job. I interviewed a software engineer once who had never written any software. He administered contracts of the third-party companies who wrote tests for the software written by other third-party companies. He'd been in this job long enough to forget his CS classes, and had gained no relevant experience. Needless to say, he didn't get the job. I sat near a software engineer who wrote code for a ridiculously small microprocessor for embedded devices. It was so small, its program counter was a feedback shift register, not a counter (fewer gates). Branches and jumps could only target every 16th storage location. There was no macro assember that could emit code for this processor, so you coded it in by hand in hexadecimal bytes. He worked at this job for five years before getting disgruntled, and found he had zero relevant experience anywhere else on the planet. I am personally one of the planet's leading experts on functional RAM testing. Guess how many jobs there are in that field.

Change jobs if you are being paid less than friends make for the same work. HOWEVER, don't change jobs because you heard that's the way to pump up your salary. If you are underpaid and have hot skills and are just a couple of years out of school, job-hopping might work for you once or twice. But in the long run, you can't pump your salary infinitely high. HR directors meet in groups or exchange emails or subscribe to reports that tell them what software developers with so-many years experience are making. The only things that will improve your salary in the long run are visible accomplishments, years of experience, and having a variety of very up-to-date skills.

A little humility goes a long way when thinking about changing jobs. Don't change jobs because you heard a story on the internet that some guy one year out of school is pulling down $150k. It doesn't mean that you can get that much (even if you believe stories you read on the internet). Freakishly rich companies like Facebook and Google do pay better than regular companies. Facebook and Google are correspondingly more fussy about who they hire. You may not make the cut, even if you think you're a smart guy.

What I Hate About Agile

Blasphemy, I know. Lest the keyboard burn my fingers, I should say that I really like Agile a lot. What I don't like about Agile is that it means whatever you choose it to mean. Agile is a brand name, useful for selling books and expensive consulting. But it isn't a specific methodology. Consultants and Agile boosters use the Agile brand the way terrorists use the al Qaeda brand, to make whatever agenda they happen to be pushing sound more powerful.

If a dev organization uses any Agile practices at all, and delivers software at the end, its Agile boosters or Agile consultants proclaim, "See, Agile works." If the organization uses Agile practices and the software is late, broken or unusable, they say, "We just weren't Agile enough!"

Because Agile doesn't mean anything, it's easy to defend any practice, no matter how lame, by calling it Agile. Don't like being held to a schedule commitment? There are agilistas out there saying, "You can't estimate schedules anyway so Agile doesn't do schedules." Don't like specs and planning documents? There are Agile boosters saying, "The code is the documentation." Rather jump straight to coding instead of doing design? There are Agile-branded supporters for that too.

Particularly disingenuous are allusions by agilists of a certain stripe to the Standish Group's CHAOS reports and the "Software Crisis". Standish makes money by selling an expensive annual report which perports to show that most software projects are delivered late and over budget (which mean the same thing). This particular strain of agilists proposes to solve the crisis by refusing to schedule. That might cut it on the web, but anywhere hardware must be manufactured or media ordered to coincide with release, refusing to schedule just won't satisfy stakeholders. These same agilistas like to compare their brand of Agile against a straw man comprising the worst habits of traditional development. This doublespeak put me off taking Agile seriously for years.

I am also leery of developers who claim to be agilists, but whose agenda is to discard any rigor or discipline in an existing software process in favor of blasting off in a blaze of coding glory. There are Agile processes that don't do up-front design, and others that don't do documentation, scheduling, or whatever. These devs take this as an excuse to do away with all these things, with predictably appalling results.

I am also suspicious of one-size-fits-all prescriptive agile processes like XP. Pair programming (a required practice in XP) is helpful, but very expensive. A mature team can be successful with a less expensive feedback method; code review or maybe static analysis. XP may be the perfect process for a particular team in a particular company. It's just not perfect for every team at every company.

Most Agile shops I've worked in had decayed Agile processes, announced with great fanfare by Agile boosters some years before I arrived, and since collapsed to a few residual practices like feature request sticky-notes on the wall, or daily stand-up meetings. But the "sprints" were ten week marathons, and the code only shambled out of QA into customer hands a couple times a year. I'm sorry, but this isn't Agile. Not capital-A Agile, and not little-a agile either.

I think this situation is sad, because if every dev team combined rapid iteration with feedback from design review and test, plus the amount of rigor in other processes that was appropriate to the quality desired from the work products, we would live in a world of far more beautiful software than what we see today. I don't know what to call this methodology. Sure it's Agile, but it's Agile in a specific way.

What I Like About Agile

As an Old Hand, I've developed software under both Agile and traditional process methodologies. The traditional process enumerated requirements, architected the system, and produced the code as sequential phases. Each phase produced a document or artifact for formal review. Since the skills needed for these three phases are actually somewhat different, the various phases could even be done by specialist teams.

What I liked about the traditional process was that it was thoughtful and disciplined. Reviews at the end of each step ensured that all stakeholders agreed that the step was complete and the project was still relevant before the project was allowed to progress.

Several things were uncomfortable about the traditional process.

• The great bulk of the work took place during the coding phase. There was not an obvious place to review the relevance of the whole project during coding.

• Too often, the team only discovered it had missed a requirement or failed to do some critical bit of design deep into the coding phase. The end-of-phase reviews were good at checking for incorrect requirements or design issues, but too often failed to catch missing ones. By the time problems were revealed in coding, the project had spent hundreds of man-hours on design or coding that had then to be discarded and re-done.

• The team could not adapt its practice to improve the project because feedback only arrived at the end of each phase. End-of-phase reviews only helped improve the next project, and only if the team stayed together.

In my humble opinion, the Agile revolution brought just two new thoughts to software design.

• Performing many small iterations of the requirements-design-code cycle instead of one big one makes feedback available earlier in the project. This gives the team a chance to improve their process to reduce cost, decrease uncertainty, and improve quality.

• If partially completed software has any functionality, it can be released and start to earn value at once.

A host of specific practices like feature cards, test driven development, or pair programming have arisen, and been packaged into name-branded methodologies like Extreme Programming. But specific practices are not what makes a project Agile. Frequent iteration, frequent release, and frequent feedback make a project Agile, in my opinion.

Agile projects do the same three tasks (requirements, design, coding) that traditional projects do, notwithstanding certain agile extremists who say they don't. A sound Agile process adopts practices that ensure that all these activities provide early feedback, and that the team acts upon this feedback.

Agile Practices Reviewed

I was so horrified by the waste I perceived in prescriptive agile methods like XP that I came very late to the agile party. Here are some specific Agile practices about which I have thoughts.

• Pair Programming: Pair Programming provides great feedback to individual developers on code quality, but it is very expensive. It's good for teaching inexperienced devs to code, but not so helpful when your team is already competent. Code review provides much the same benefit at lower cost.

• Code Review: There are automated tools for code review that present the user a visual diff of the modified code, and allow commenting and approval. While these tools are great for reviewing point-changes during maintenance, they discourage thorough review of whole interfaces. It's too easy sitting at your desk alone, to take a perfunctory glance at the changed lines, and say, "Looks good. Ship it." Shops using automated review tools have to keep an eye open to be sure all reviewers are taking a decent amount of time to actually read the code. I have had good results from a heavy-weight design review involving actual meetings. The formality and required prep work for the meetings, and the expectation of face-to-face feedback from peers induced higher quality even before the review.
  
  Linters and static checkers are tools you can use for code review too, even if you're only reviewing your own code.

• Unit Test: I love unit test. A good set of unit tests remove much risk from changing software. Unit tests written alongside (not after) developing the code form a check on the consistency and completeness of newly designed APIs. The needs of unit testing focus attention on separation of concerns and isolation of dependencies, which both push up the quality of the resulting code. Unit tests are most effective when devs buy into the idea of testing as you go. If writing the unit tests is just an annoying check-box to a developer, they are unlikely to give it the attention required for a really good result.

• Code as the Only Deliverable: Some agilists suggest that, since the code is the only artifact shipped to customers, no other artifact has any value. Production of requirements lists, design documents, schedules, and other non-executable artifacts should thus be viewed as wasteful.
  
  This advice has merit from an aspirational standpoint, but has practical weaknesses. Code is too voluminous and precise a language for expressing requirements. Code is too low-level to express architectural decisions. Code cannot express schedules at all. Other kinds of documents may be necessary for internal communication and review, even if they aren't delivered to customers.

• Schedules: There is a persistent myth that Agile methods don't do scheduling. This is untrue on many levels.
  
  At the micro level, the effort for individual features must be estimated, and big features broken up into sprint-sized pieces (if you're doing sprints. But otherwise you're doing big-bang development. Hisssss).
  
  At the macro level, Agile projects must do scheduling when stakeholders require it. This happens any time they interact with other projects, when long lead time hardware and mechanical packaging must be designed and ordered, or when the dev organization must release by a calendar date or forego important sales (in time for Christmas, for instance). Only a small subset of Agile projects can safely ignore macro scheduling issues, or refuse to predict completion.

• Refactoring: The world of software development is of two minds on the subject of refactoring. One school believes that any change to released software must be minimal, lest the change introduce bugs. The other school says refactoring is OK if it adds value by making the software more maintainable or more flexible, or better supports new features.
  
  In my opinion, both thoughts are schooled by experience. If the initial design and coding were weak, and there are no unit tests, then any change is risky, so change must be minimized. If, on the other hand, the initial design was well motivated and good unit tests are available, refactoring just makes things better and better.