Talk About Quality

Jesse Gibbs at Atlassian sent me to the following post from Scott Bilas at the game software company Loose Cannon Studios. In Peer Code Reviews: Good Commenting Practices, Bilas says code reviewers should seek architectural issues, and adherence to good software development practices and coding standards. And that they should look for mentoring opportunities. At the same time, he lets them off from other responsibilities, saying “Reviewers aren’t expected to catch everything,” and, “Reviewers aren’t expected to catch deep or systemic design problems.”

It’s a pretty good tutorial on the current best practice of code review. So why does it feel like something big is missing?

Upon reflection, it turns out that, unlike, say, editing and commenting on a book, code review is not really a reviewer-author relationship at all. An editor may make a lot of changes, but those changes end with publication. Code, on the other hand, will be fixed, extended, and refactored by future developers. Each future, or “next” developer will need exactly two things from the code in order to do his or her job: readability and understandability. Code is readable if it has the right balance of abstraction vs. detail at each level of the code. Code is understandable if the reviewer can, without help from the author, see what the code does, and why. If code is clear, that is, readable and understandable, modifications will be easy and error-free.

So the question “what are the responsibilities of a code reviewer” turns out to be a trick question. Like this one, from driving class:

Question: In the following traffic diagram (imagine any diagram you want), who has the right-of-way?

Answer: Nobody has the right-of-way. Right-of-way can never be possessed, only given.

The code reviewer does not have any review responsibility. The code author has all the responsibility to write clear code, and provide it to the code reviewer. We should stop calling that person a “code reviewer”; instead, say “code user”.

Code review becomes a simple and easily explained two-step process. First the “code user” reads the code and notes down what s/he doesn’t understand how to use, and second, s/he meets with the code author to present what was not clear. In that manner, the “code user” successfully stands in for the “next developer”, and gives the code author an early chance to make things better. As a bonus, all the things a traditional code reviewer was supposed to check, or at least the important things, will be checked. Mentoring will happen. All this when we realize that code review is code use.

Google’s “Mr. Automated Testing”, Miško Hevery, talks about Software Testing Categorization. From the title, it doesn’t sound like much, but it prompted a lot of good discussion. Some excerpts on key points, edited for length and clarity:

Know what you’re talking about

You hear people talking about small/medium/large/unit/integration/functional/scenario tests but do most of us really know what is meant by that? — Miško Hevery

Need for speed

Let’s start with unit test. The best definition I can find is that it is a test which runs super-fast (under 1 ms) and when it fails you don’t need debugger to figure out what is wrong. — Miško Hevery

Make slow tests faster by running them in the background on spare machine cycles. Works for static analysis tools too. For example, Riverblade’s Visual Lint add-on for Visual Studio and Gimpel Software’s PC-lint. — Talk About Quality

I heard a senior test manager present at a conference who espoused the sophistication of his automated test regime (on a mainframe no less). Countless tests ran every evening, unattended. Lots of clapping in the audience. In the break I met a guy who worked for the presenter. He asked, “do you want to know why the tests run so fast? We took out all the comparison checks”. — Paul Gerrard

You’re right to have a healthy concern. Whether automated tests run in a millisecond or in several hours, they always have to be re-reviewed to see if they test something useful. Passing tests are especially dangerous. Everyone thinks green is great so they don’t look at the tests to discover that features have moved on and the tests don’t test anything. Automated tests are like automatic shift (does anyone still drive stick like I do?): very nice and we take it for granted, but when you press on the gas you still have to look where you’re going so you don’t cause an accident. — Talk About Quality

Test Planning applies to automated testing too

At my previous employer, the standard unit tests took upwards of an hour (for fewer than 2000 tests). This discouraged developers from running them, which resulted in broken builds for everyone. When we worked on fixing this, mock objects provided most of the salvation. However, a good portion of the tests were really integration tests and by correctly identifying them as such, we were able to remove them from the standard suite, and instead run them nightly. What Misko is missing from his post is how test classifications go hand in hand with scheduling tests. — TheseThingsNeedFixed

Thanks to Bulkan Evcimen for tweeting about an article I doubt I would have found otherwise: “A Genetic Programming Approach to Automated Software Repair” (pdf link here), by  Stephanie Forrest et al. They describe there how they fed a genetic programming algorithm the following inputs:

1. Source code with a known defect
2. A negative test case
3. Several positive test cases

The authors show how software with the genetic algorithm is capable of repairing the defect such that the software under test no longer fails, while it still passes the positive test cases. They complete the activity with additional software that reviews the change and reduces its scope to the minimum.

Is this the end of the maintenance programmer? If so, this time there would be a happy ending: John Henry lives, and goes on to be a railroad engineer.

It’s Thom Holwerda — keeping things simple for us:

Simple Code Quality Metric

Here’s his site — with a search for “code quality” (warning: if you can’t ignore “Evony” web game ad graphics, stay away).

A medical radiation machine operator types the letter ‘x’, realizes it’s an error, backspaces, types ‘e’, and continues. Consequences of the error and related defects lead to the patient’s untimely death.

Ten years later, a Japanese stock broker mistakenly switches the share price and amount on the sell order of a new stock. He tries to cancel the order, but fails. His employers lose $225 million, and are involved in lawsuits for years.

A blogger clicks the “back arrow” by mistake, gets a warning message, concludes he meant to continue writing, and clicks “OK” to continue. He loses his work, and has to rewrite it.  Here, the consequences are annoying, if trivial by comparison.

What do all these cases have in common? Canceling a request. If it’s not hard enough to program computers to do what we want them to do, who would have thought that telling them not to do it would be hard too?

The cancellation scenario — or “use case” as it’s called in software design — is the silent partner of every positive request supported by a piece of software. It has to cover giving the user clear options, executing the cancellation, and rolling back any partial results. Things get more complicated if authorization is required, or if the transaction has already gone through (both of those requirements figured into the Japanese broker error story). Cancellation and rollback are also part of automatic requests that may occur if one software module (the “server”) cannot complete a request by another (the “client”) and has to make sure to put everything back the way it was, and send the proper response code.

So the next time you’re designing a piece of software, no matter how simple, think what it’s supposed to do, but also what it will do if the user or client calls out, “Cancel that!”

You can do anything in software. Hordes of eager computer science graduates, not to mention brilliant open source coders, keep joining the ranks of software development because it’s fast and fun.

The fact that software is instructions to a machine (the “hard” in “hardware”) seems to have been the only thing John Tukey had in mind when he coined the word “software” back in 1958. (Dr. Tukey, an accomplished statistician, was more focused on computerizing the Fast-Fourier Transform, and criticizing the Kinsey Report for its questionable sampling methods.)

While software attracts developers with its ease in creating things, it tempts us all with its other “softness”: amenability to change. Software can be endlessly fixed, extended, improved. And that advantage demands something of developers which was unexpected, and, well … hard.

Hardware must fit form to function so it’s easy to use. And come with good documentation for maintenance and repair. But that’s all on the outside. Software, always ready for change, has to be clear and readable on the inside too. In other words, software developers have to be good writers, because the next developer will have to read and quickly understand what’s going on in order to change it.  Written changes, again, have to leave the software in a clear and readable state.

In high school, I avoided English class like the plague (and got bad marks for using cliches in my papers), preferring to go into school on snow days to use the (one) computer. Good writing is not why people become programmers. But it’s exactly what we need. Clear written communication. Equal in impact to life-changing books (pen mightier than sword and all that), software crucially affects our lives — from cars, to food transport, to the electric grid.

That good writing is an unwelcome requirement of sofware is why developers code quickly and obscurely, hate documentation, and shun code review. And  why managers push for features, delivery, and fixes, while devaluing internal quality.

Is there hope? The only one I can think of must exploit other likes and dislikes: managers want software changes fast, while developers like writing new code more than fixing someone else’s (or their own) bugs. Good writing is the only way to make code support that scenario, and reap the advantages of software.

I know what I want: highlight-and-comment capability in a text editor. And since plain text really shouldn’t be marked up, I’d want it to save the comments in a separate text file, with line numbers of what was commented on, and the comments’ text. I’m not the first one looking for this capability: “climbanymountain” asked about it on Joel on Software back in 2006: “win32 text editor for code review?“

I started thinking about writing a plugin for some popular text editor, such as Notepad++ or Notepad2 (the latter is my favorite for simple editing). But before the “build or buy” decision, I thought I’d ask around the office.

A lot of ideas, but here was the simplest. Light and off-the-shelf. Use a diff tool. 

The following procedure works fine, and “supports” any version control system. In my example, I’m using open-source WinMerge as my diff tool, IBM Rational ClearCase as my version control system, and Microsoft Windows Explorer as my tree browser. Say the source file to review is called “MyFile.c”, and the reviewer’s name is Firstname Lastname.

The parts in square brackets are optional: omit them if you don’t want to clutter up your version control system with code review records, but just want to use them to prepare for sitting with code author.

1. Browse to the source code file I want to review; [check it out and] open it.
2. Immediately Save As MyFile_flastname_review.c in the same directory. Close the source code file.
3. Select the source code file and the review file (they are next to each other in alphabetical order), right-click and open WinMerge on them.
4. Go to the pane of the review file and type review comments anywhere I want. Save regularly with CTRL-S. (Optionally, for better syntax highlighting, can precede comments with comment character such as “//”. But don’t have to.)
5. Generate a diff (patch) file, call it the same name as the review file, but add .diff on the end. (In WinMerge, that’s on the menu with Tools > Generate a patch.)
6. [Check in all 3 files (adding 2 new ones to source control) -- check-in activity per defect tracking system, and comment with one-line summary of the review.]

From this point, code review comments are easily viewable side-by-side with the code by viewing in WinMerge or similar viewer.

Also, the diff file clearly identifies the line number commented on, and the comment text. I figure I could write an AWK script to turn that into CSV, for tabular display and tracking in Microsoft Excel, if I wanted to.

Having a standalone consumer application get stuck or crash, requiring reboot, is not the worst thing that can happen. (Worse is incorrect behavior that causes data loss or physical harm.) But requiring a reboot is the most annoying failure in non-safety-critical systems.

If there’s any good news, it’s that the list of fault modes is short:

• System resources exhausted
• Mistakenly idling
• Waiting for acknowledgement that never comes
• Deadlock

Did I miss any?

Only exception-safe code can avoid these undesired end states.

Design by Contract (DbC) is one way to exception safety.

Failure mode and effects analysis (FMEA) helps you plan a path to get there.

That’s right, not “development” but “developer”.

The latest issue of The Embedded Muse newsletter—#179 (pdf)—has a reference to an interesting and very different kind of book about C. It’s about the business pressures and the thought processes of C software developers. And a lot more. It’s also long—1600 pages! So download a copy and skim for what interests you:

The New C Standard: An Economic and Cultural Commentary

Some lines that caught my eye, just in the first 100 pages, skimming with the “Page Down” key:

“Software developer interaction with source code occurs over a variety of timescales [for example, those] whose timescale are measured in seconds.”

“The act of reading and writing software has an immediate personal cost.”

“Source code faults are nearly always clichés; that is, developers tend to repeat the mistakes of others and their own previous mistakes.”

“A list of possible deviations should be an integral part of any coding guideline.”

“The following are different ways of reading source code, as it might be applied during code reviews …”

Other related references from the book’s author Derek M. Jones (http://www.knosof.co.uk/):

• Blog: http://shape-of-code.coding-guidelines.com/
• Article:  Coding Guidelines: Fact and Fiction
• Article: The 7±2 Urban Legend (small pdf)

And somehow related, from another author, Les Hatton (http://www.leshatton.org/):

• Article: Testing the value of checklists in code inspections

Consider a function in software code, and how to write it properly so that it meets its requirements and doesn’t fail. The standard way of looking at it is that a function has an API — a signature — of input and output variables and types. The code for the function has a pretty standard form. There are checks for invalid input, and a function body which implements a mini-flowchart of conditional checks, and maybe a state machine if needed. All this to take every given input vector to the correct output.

Unit tests, whether they’re written before coding (test-driven development), or after, apply test input vectors to the function and check if the expected output is produced. The same story applies to a module: a collection of functions or classes that implements a feature. There are still valid inputs to check if they give correct output, and invalid inputs that must be rejected. Best-practice test design methods, such as boundary-value checking and equivalence classes, help the developer choose the best test vectors from the sea of possibilities. The ones that will produce good tests that cause failures early, while the code is still in the hands of the developer.

So why do we still have so many “surprise” failures? Unit-tested code that nevertheless comes back from system testing with failures? And the Steps to Reproduce — so simple — like, “I left the system running overnight and when I came back in the morning it had crashed.”

The answer may be in the flow of data. Functions and modules in embedded software don’t just deal with single inputs one by one. Generally they are expected to process streams of data in real time. For example, a modern television set-top box, with a built-in disk, has to process multiplexed video data and metadata, the same from the hard disk, as well as a much slower stream of user input via the TV remote control. If the code slips up, or leaks memory, sooner or later, you get a wrong, stuck, or crash situation that QC proudly reports.

Where have we seen this before? How about those robot car competitions that university engineering departments often hold? Contests which have expanded outward to worldwide challenges to design, for example, an auto-piloted car. (See “The DARPA Urban Challenge“). These contests demand software that will keep a driverless car on track for long periods of time, avoiding stationary and moving obstacles (other cars).

Could it help, then, to see a lowly function in embedded software not as a flowchart of if-then-else statements, but as a little car — or perhaps delivery truck– that must be kept on track, avoiding obstacles in the data stream while delivering packages to the right place? How would that view affect code layout, code review, and unit test design? See you at the track!