One-Man Code Review

How do you do a code review if you are working alone? You obviously have already read your own code and found it beautiful. Code review fits into the development process as one means of getting feedback on the quality of the code you are writing. Basically, you increase use of all other tools that give the same kind of feedback.

• Turn the warning level of your compiler all the way up. Do a release build if you're using Visual C++ because the release build reports more errors.
• Install and run a static checker. CppCheck is a free static analysis tool. Coverity is a paid tool. LLVM has a static checker, or so I hear. If you have an expensive version of Visual Studio, there are static checkers, but I've never gotten to use one.
• Memory checking tools that find frees of invalid or already-free storage are built into visual studio and available for free for linux, and there's always valgrind if you're patient. 
• Write and run a set of module tests.
• Profiling tools produce results that may cause you to rethink your interfaces. Writing module tests forces you to use the API's you create, which often causes you to change things.

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.

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.

Coder's Block

I used to get coder's block, which is like writer's block, only for software. I'd stare at an empty screen, having no idea where to start. I'd dither between two implementation directions, unwilling to commit to either course, while the cursor blinked away the seconds in the corner of that empty screen.

I cured my coder's block, by the same technique recommended to cure writer's block; write something.

OK, initially this is hard advice to follow. By definition, if I didn't know what to write, what could I write? If I didn't know which implementation path was best, wouldn't I potentially be wasting my effort writing the wrong thing?

The solution that worked for me was to write something different. If I didn't know what code to write, I would write down a list of requirements (in prose) for the code. If I was looking at a big undefined area and I didn't know just what I'd need, I would write a structured walkthrough, again in prose. Or, I would write the function header, so that what I was doing had a name and an argument list and a return value. If I was feeling particularly virtuous, I would write a module test attempting to use the unwritten method to solve the problem.

And this is what happened. As soon as there was something on the screen, I could begin to review it. I might realize that I didn't know what the requirements were. That gave me something to think about, and pretty soon I did know. If I did a walkthrough, I would find myself writing in the passive voice, which would mean that I didn't know what object called which method. Now I had a hook on which to hang further thought. If I wrote the function name, I could begin to think about whether I really needed to do a thing with that name. Every word I wrote got thoughts out of the nebulous fog inside my skull and down in (temporarily) solid and immutable text where I could see it and analyze it and refine it.

If I couldn't write a paragraph, I'd write a bullet list. If I couldn't write a bullet list, I'd write a few conceptual phrases. What I would not do is get a cup of coffee or read my email or surf the web. That way lay madness. OK, maybe just a little surfing, but I would hold it in mind that I was procrastinating, and what I needed to do was to grind it out.

I know some people who deliberately distract themselves with some email or web surfing. Maybe that works for them, or maybe they are self-deluded. But for me doing something else is just postponing the hard stuff. I know some people who say the answer comes to them in a dream. In my dreams I just worry about being blocked, although sometimes in the morning I have amazing insights in the shower, when I'm very fresh.

When I grind it out, I almost never find I've gone down the wrong road. Perhaps that's part of being an Old Hand. Come to think of it, I conquored my coder's block about the same time I became an Old Hand.

The Trouble With Lint

You all know what lint is; those little bits of white fuzz that stick to your clothes. Lint isn't exactly dirt. It mostly just makes your clothes look untidy. Mostly.

Software has lint too; those compiler warnings you see every time you do a clean build. You've seen them a thousand times before. You know they're harmless. If they're in your code, maybe you think, "I need to fix that someday. But not now."

This posting is about why it is important to remove lint from your software sooner rather than later.

You see, lint has a dark side. Enough lint in the same place is flammable; sometimes even explosive. Houses burn down each year from fires that start in the clothes dryer.

Software lint has the same nature. Too much lint; too many warning messages; and you stop paying attention. Then when an important warning appears, you don't notice. At a previous employer, a team of half a dozen engineers spent four 16-hour days (including Saturday and Sunday) hunting a bug that held up an urgently needed release, when the problem they were hunting was printing a visible compiler warning. It was one of 200 such warnings, so nobody paid any attention.

I was going through some code today, removing lint. I came across this warning.

Foo_UnitTest.h:178: warning: right-hand operand of comma has no effect

Anyone know what this means? Here's a hint; the code at line 178 read

delete [] buf1, buf2;

Still confused? Here's an explanation. C and C++ share a little-known feature called the comma operator. The expression <expr1> , <expr2> evaluates <expr1>, then throws the result away (except for side effects), then evaluates <expr2>.

I imagine the person who wrote this line of code intended to delete the storage pointed to by both buf1 and buf2, but that's not what happened. Instead, the expression delete [] buf1 was evaluated, then discarded (except for the side effect of deleting buf1), and then the expression buf2 was evaluated. So buf2 didn't get deleted.

If you say, "Well, it doesn't really matter because it's just a unit test." then you are entirely missing the point. Removing lint is a discipline that results in clean code that runs the way you expect. If buf2 has a non-trivial destructor, there's no telling how much code isn't getting run (and therefore isn't getting tested) because buf2 isn't getting deleted. You wouldn't know that without investigating what the type of buf2 was.

Even lint that doesn't affect the generated code, like unused variables, can be a problem if these unused variables confuse a future reader of the code. It's easy to comment out this code if you want it to stay in the listing. Easier yet to delete it.

C++ provides syntax for defining functions that don't use all their arguments.

void classname::func ( int parm1, int /*parm2*/ ) {...}

defines a method that takes two int arguments, but only uses the first one. Sometimes you need this for defining base-class methods meant to be overridden in derived classes. Commenting out the argument name turns off the compiler warning, and clearly expresses your intent.

Sometimes your compiler insists on issuing a warning for code that you know is ok. If you can't fix the code with an explicit cast or something, the language implementation probably provides a #pragma for temporarily turning off the warning. You can go even further, running a static checker like lint(1) or cppcheck and fixing the code to remove the warnings it produces. There's just no reason not to have a clean build, and every reason to have one.

Learning About Robustness

Something I think about from time to time is, "What do Old Hands think about code, that is different from what rookies think?" Here are some examples.

• When I was a rookie, I believed programs should be precise. If the program parsed some input, that input should conform. I believed that a program was robust if it printed a detailed error message and halted, so I could quickly fix the input.
  
  As an experienced developer, I realized that processing the input was far more important than ensuring that every comma was in the right place. I came to believe that a program was robust if it accepted a generous superset of the expected input syntax. I remember hearing this advice as a rookie and snorting with derision at the thought of anything so sloppy. This is another difference between Old Hands and rookies.
  
  When I became an Old Hand, I realized that any program you run frequently provids a service. Any time the program halts prematurely, it fails in its reason for being, which is to provide the service. I discovered that even a program that is not working correctly may be more useful than a program that won't run. I learned that a program is robust if it provides its service, all the time, under the widest range of conditions. A robust program should not fail, and if it does fail it should recover, and if it cannot recover, it should restart, and if it can't restart, another program should restart it. The code to provide all this robustness amounted to as much as 50% of the total, but I no longer viewed that as wasteful excess.
  

• When I was a student, I never checked return codes. "How could a system function possibly fail?", I thought. I was naive enough to assume that the people who write system functions never made mistakes, and hardware never failed.
  
  When I was a rookie, I discovered that even if a function failed only once in a million calls, I would see it fail. A million calls just doesn't take that long when a program is running flat out all day. Such a program would terminate unexpectedly, usually saying no more than "segmentation fault".
  
  As an experienced developer, I learned that functions fail all the time, but I hadn't known it, because I wasn't checking the return codes! Usually functions failed because the argument values were illegal. I remembered spending hours looking for why my program wasn't working, when the functions were telling me exactly what was wrong. I meticulously checked every error return, and reported failure up to higher levels of the code. I also began writing functions that did more checking and logging, because when these functions failed, they told me what I was doing wrong.
  
  Once when I was old enough to know better, I wrote a library of meticulous functions tthat checked every return code, to check out some functionality in Windows. It took days. A colleague who was an Old Hand bodged together an informal but usable tool in a couple of hours because he didn't check return codes. I learned something that day.