Josh Kerievsky called me to task for not following up on his third version of his story of providing a quick fix before a test-driven fix. As I said, I don’t want to be critical of Josh. I will, however, cast a critical eye on the code he has shared.

For those just tuning in, the previous installment talked about differences between the culture we espouse and the culture we practice. This installment will return to that.

Reading the Code

To start, I found the code snippets hard to understand. They require knowledge of parts of the system that are not visible in Joshua’s blog post, and likely not visible on the screen when programming, to understand. Self-documenting code is an ideal that is perhaps never reached, but I think it can be better than this. As of this writing, the only explanatory help I’ve received from Josh is that getStringParameter(COMPILATION) is getting a value from the HTTP request.

Notice that a private method is calling this and also sending it off to another method. There is no validation, other than null checks, on the input received from the network. This sets off warning bells in my head. I immediately think of a request for an album named “SQLinjection;drop table albums”. Perhaps the Data Access Object protects against this.

In terms of readability, maintainability, and testability, though, I like to clearly separate the input, processing, and output. Sometimes this separation is achieved by using different methods, and sometimes by using whitespace. In webapp action classes, I generally have a block of code that unmarshalls the request to domain objects, likely a parameter object that gets passed to the domain logic. While the framework has already unmarshalled the string of bytes into HTTP concepts, including a hash of parameters by parameter names, this is only half the process. It’s as far as the framework can take it, but I’d rather take it further and express the parameters in domain terms instead of simple Strings.

In this case, the domain parameter seems to be the location within the library of the current selection. This location is what will be bookmarked, and includes values representing the programming language, album, and page within the learning library. If we create a Bookmark class to hold these values, then we can include convenience methods for building a Bookmark from the HTTP parameter, and also for creating URL parameters to have consistent processing when we get to the output stage. We can also ensure that we create a valid Bookmark, even if some of the parameters are missing or bogus.

The output stage of a webapp returns the page to be sent to the user’s browser. Often this is done using some sort of templating library, and data values will be inserted into the template by the library or web framework. To do this, the data values must be placed into a scope where the templating code can access it. Then the frameworks output function is called.

In between these two are domain processing. From the example code, this processing includes persisting various values associated with the user–programming language, current locator, bookmark. The first two sound redundant to the last, but perhaps there are nuances that are not expressed in the names.

Now the computer doesn’t care if you mingle the input, processing, and output into arbitrary boundaries of code. I find it makes a difference in understandability, though. Of course, if you’re working in only one codebase and doing it frequently, you get used to its quirks and don’t notice them so much.

I’m looking at the code as an outsider, trying to understand it by reading it. In doing so, I inevitably compare it with patterns of code I’ve seen and written in the past. Some of these patterns I use to make the code easier to understand. Some of them I use to make the code less likely to have patterns. There’s a large overlap between these two subsets.

Programming Friction

As I read the code, I find I encounter friction. This is an idea I got from Janelle Arty Starr, though I interpret it a little differently than she does. Friction is the difficulty in figuring out what’s going on in the code. The more time spent in friction, the more painful the programming process. I don’t think there’s such a thing as perfect code, however. It’s humans writing it and humans reading it, and both contribute to the friction.

I encounter friction when reading my own code, especially after I haven’t touched it for awhile. I have learned some tricks to reduce the amount of friction I face. Some of this is in the expressibility of the code. I once had the pleasure of revisiting some assembly language code I hadn’t even seen in over a decade, and was delighted at how little friction I encountered. Another time I found myself fixing the same bug twice, months apart, because the code was too convoluted.

TDD

One of the techniques I’ve learned to use to reduce friction is test-driven-development (TDD). I write a test as I’m thinking what I want the code to do next, and then make the code do it. Not only does this catch implementation bugs earlier, but it also puts me into a different mindset about the code I’m writing. I find it easier to notice when the design is accumulating friction, and I do something about it.

I work both from the business function level, writing an automated acceptance test for externally visible behavior, and at the programmer level, writing microtests for my expectations of the code as a programmer. Doing both seems to keep me out of a lot of trouble and really speeds up my software development.

In terms of friction, the business level tests remind me why I was doing something and the programmer level tests remind me how it’s expected to work. I still try to make my code as expressive as possible, but the tests give me additional information. In fact, the tests put me into a mindset to focus on the expressibility of the code as I go.

When I look at this code, it doesn’t look test-driven to me. For one thing, the methods are private, which gets in the way of testing them in isolation. Also their names don’t quite match what they’re doing. This is something I notice when I’m writing tests.

It also seems that it would be less likely to create methods that do multiple things. Would you write a test that checks that displayPage() sets properties on User?

It could be that they’re tested only from the business level. Trying to work only from that level creates testability problems, as you might not be able to notice from the outside how to create an edge condition within the code. In fact, you might not notice there is one.

I doubt that this code has those high-level tests, though. If it did, surely “David,” who is “extraordinarily careful to always practice TDD when working on any production code,” would have written a test to demonstrate the bug and make it easy to troubleshoot and verify a fix. If there are no existing high-level tests for this functionality, though, it’s considerably more work to set up the first one. There would be a period of friction trying to understand how to get this existing code and the test framework working together. It’s easier when they’re born at the same time.

Refactoring

Another bit that suggests a lack of programmer-level tests is the “refactoring” that caused the bug. Clearly this wasn’t a true refactoring, since it DID change behavior. When going from
String albumId = getUser().getCurrentAlbum().getAlbum().getId();
to
String albumId = getStringParameter(COMPILATION);
the test setup has to change. It has to move the albumId value from deep in the User object into the HTTP request parameters. This is a big clue that it’s not a refactoring.

The Fix

Since the offending change was only two days old, I would have been tempted to just revert back to the version that had been working. This is a more sure step than fixing the code. Perhaps, though, a lot of other changes had been committed in those two days. I wonder if any of them had bugs introduced during that time? Bugs tend to travel in groups, especially if the programmer is tired, distracted, or over-confident.

The Culture

Clearly the software development culture in this case is not as test-driven as they think of themselves. It’s that mismatch between the cultural story and the cultural behavior that’s the most serious problem, in my mind. A lot can go wrong if you lie to yourself.