Monday, March 10, 2014

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.

Thursday, March 6, 2014

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 crash, becaue my employer collapsed. It was hard to find work because it was the 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.

Sunday, March 2, 2014

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.