Forgotten Refactorings

Shooting pool at City Billiards in 2002, my friend Hans offhandedly says, "You should read Martin Fowler's Refactoring." That moment changed my life. Well, technically, the hours spent reading the book over the following week changed my life, but let's not quibble.

Seven years later, I still believe this is the book to read when starting out on the path to being an engineer. It's not so high-level or inaccessible as Design Patterns (although still a foundational book), nor is it as low level and implementation specific as Clean Code or Implementation Patterns (also worthwhile reads).

At the time I read Refactoring, I was neck deep in PocketPC and PalmOS development. C and C++ code without unit tests. Difficult to write and difficult to change. The better developers knew the operating system API by memory, and the best developers could explain how the API interacted with the hardware. I remember one job interview where I was asked what "Hello" + 4 yields.* My answer of "I would never write code like that" was met with, "Of course you wouldn't, but you'll need to debug it." How true.

Refactoring laid out a vision of good, clean code that went beyond simply working correctly. This wasn't exactly new information. My peers and I all valued short methods, small classes, high cohesion, and low coupling even if we had a lot to learn about how to achieve those properties. But we also valued shipping code on the delivery date and not working the weekend too frequently. So when timelines got tight, we cranked out sort-of working code and moved on to the next project, which was usually the maintenance phase of the same project.

The beauty of Refactoring was that it laid out specific instructions on how to make crummy code better. And those instructions were almost mechanical in their execution... first do this, then do this, then finish. In fact, the instructions for each refactoring are in a section called "Mechanics". The promise was: if you follow the instructions then your code will improve and nothing can go wrong. I loved this. Finally, I had a guide and body of knowledge around what to do about all that old code I had to sort through.

So why did it all go so wrong?

Why does refactoring so often introduce bugs into your system?

Why is refactoring a word you can barely mention to QA and Operations without horrified reactions?

Most importantly, what happened to the "and nothing can go wrong" part of the refactoring promise?

I'll tell you what went wrong: our tool vendors screwed us.**

I've been interviewing Java candidates all week, and I always ask about refactoring. Everyone knows what refactoring is. Many people know the keyboard shortcuts. It couldn't be easier to recall the various refactorings available in the IDE... there is an entire freakin' menu of them. And not a single one of those menu items is implemented correctly in terms of Fowler's Mechanics:

Chapter 1, Page 7: The First Step in Refactoring: Whenever I do refactoring, the first step is always the same. I need to build a solid set of tests for that section of code.

Page 7! This isn't buried in some appendix, it is front and center. And in case you're just skimming the book and missed it, it's called out in a sidebar with a lightbulb on the very next page:

Chapter 1, Page 8 Before you start refactoring, check that you have a solid suite of tests. These tests must be self-checking.

I don't know how much more emphasized step 1 of refactoring could be: don't touch anything that doesn't have coverage. Otherwise, you're not refactoring; you're just changing shit. And not a single IDE enforces or encourages you to have test coverage before mucking about with the refactorings. I propose we make a change to our behavior. If QA or Operations asks us what introduced a defect, and our answer is a refactoring without coverage, then we reply, "I was just changing some shit." I think this would work to eliminate reckless refactorings. Hmmm. You know what might work better? How about we just try a little harder to bring our code under test. Reading "Working Effectively with Legacy Code" is a good place to start, as well as using functional testing frameworks like DBUnit or maybe FIT. The choice is yours, I guess. The second option seems a little more useful in the long term, however, I kinda like the idea of developers telling QA and their managers that software is late because they decided it was a good time to "change some shit".

But please, whatever you do, stop saying refactoring when you're making changes to untested code. You're ruining it for the rest of us who want to do real refactoring.

* "Hello" + 4 yields 'o' as a character. Strings are just character arrays lined up in contiguous memory, so H is offset 0, e offset 1, and o offset 4. I thank God this information is no longer useful to me.

** Yeah, it's more our fault than the tool vendors, isn't it? Still, it's a lot easier to blame a nameless entity than the guy sitting across from you.

Writing (and Testing) Good Error Messages

Maybe I do spend more time on error messages than most people. I usually write some sort of unit test, which at least quadruples the time it takes to write "throw new RuntimeException()

"OH GOD NO! Unit testing exception messages, what is he thinking?"

Hear me out become calling me a pedant.

Contract-Driven Development is a technique more widely accepted than it is debated, especially for Java development. While Java does not support contracts as part of the language, the conventional wisdom I'm given is to think about what constitutes a method's contract and invariant state and then fail fast when this contract is violated.

Consider the following Java method:

public void process(Message msg) {
 Content content = msg.getContent();
 //...continue processing mesage content
}

At the most trivial level, thinking about contracts means thinking about the range of inputs. Null is certainly not a valid input to this method. One might be tempted to explicitly check for a null parameter like this:

public void process(Message msg) {
 if (msg == null) throw new IllegalArgumentException("Null: msg")
 Content content = msg.getContent();
 //...continue processing mesage content
}

This seems like a silly exception; you would have gotten a NullPointerException instead of an IllegalArgumentException, big deal. But what if the NullPointerException happened in the middle of a long process task instead of the first line of the method. Could a transaction be left half executed? What state is the system in when an exception is thrown halfway through a method? It's quite possible that the system would be left in an unstable state: a state that violates your class's invariant. So yes, I do believe we should be asserting the proper range of parameters on method input. In fact, this type of checking saved me a surprising amount of time on Friday. When we changed our web service schema, JAX-B beans started coming through our sevice with nulls in bad places. Our Flex developer was able to say, "Why am I getting a null: site exception" in our standup instead of asking, "why am I getting a null pointer at line 23 of class xxx". Good error messages are easier to diagnose.

Not only do I like to write argument checking exceptions but I also like to test them:

public void testProcess_Contract() {
 shouldFail(IllegalArgumentException) {
   service.process(null)
 }
}

This is not an incredibly helpful test and looks like the litter avid TDD practitioners like to scatter around a test tree. I write it mainly to get my testing juices flowing and to take the overhead of creating the test class. Plus it's part of my TestCase template file so it's probably just habit.

So what is this about testing error messages? As a system wide invariant, this is what I consider to be my contract with my users: Error messages should contain the cause of the problem and a solution to the problem. There is no error message on Java's NullPointerException, but "Null: message" satisfies this contract because it tells you what the cause of the error is (message) and what a solution is (don't pass null). By the way, including the failure information in the exception message is Effective Java Item 63: Include failure-capture information in detail messages. A better example can be seen in the Groovy codebase, where a method called visit requires some very specific objects to be in an array:

public void visit(ASTNode[] nodes, SourceUnit source) {
 if (!(nodes[0] instanceof AnnotationNode) || !(nodes[1] instanceof AnnotatedNode)) {
   throw new RuntimeException(
     String.format(
       "Internal error: wrong types: %s / %s. Expected: AnnotationNode / AnnotatedNode",
       nodes[0].getClass(),
       nodes[1].getClass())
   );
 }
 //... continue processing

This is quite a bit of code for error handling, largely because of my insistence on using String.format whenever I can. Anyway, if this error occurs you are told what happened (wrong types), you're told what was expected (AnnotationNode and AnnotatedNode), and you're told what was actually passed (nodes[0].getClass() and nodes[1].getClass()).

So should you test the exception message? Yes you test the contract: Error messages should contain the cause of the problem and a solution to the problem.

public void testVisit_Contract() {
 def message = shouldFail(RuntimeException) {
   def badInput = [new ConstantExpression("sample"), new EmptyExpression()] as ASTNode[]
   new SingletonASTTransformation().visit(badInput, null);
 }

 assertTrue(ex.getMessage().contains("wrong types"));
 assertTrue(ex.getMessage().contains("ConstantExpression"));
 assertTrue(ex.getMessage().contains("EmptyExpression"));
 assertTrue(ex.getMessage().contains("Expected: AnnotationNode / AnnotatedNode"));
}

Testing exceptions is fragile, I'll admit. But using assertTrue and String.contains minimizes that fragility. Here we're testing that the error is stated (wrong types), that the values causing the error are reported ("ConstantExpression" and "EmptyExpression"), and that the expected values are reported ("Expected: AnnotationNode / AnnotatedNode"). I find this testing approach to be a good compromise between not testing the contract and writing a fragile test, and the style still allows those annoying TDD zealots to write tests in your codebase while playing nicely with everyone else.

Lastly, I believe writing assertions on exception messages might fail under certain character set/localization schemes. I seem to recall this was a Java Puzzler at some point but don't have the book handy. Will these assertions fail under different character sets? I've never seen it happen myself.

The End of Intuitive

We all want our interfaces to be intuitive, don't we? Sounds good to me. I know I've used this word in the past when passing judgment on a user interface: "I'm confused by the interface... it's just not intuitive."

To intuitively understand something means it is understood without prior exposure to the subject, without requiring a learning process, and without using rational thought. Running away from a bear in the woods: intuition, fight or flight. Drinking water from a lake: not intuition, prior experience with lakes teaches you to expect water. Shaking a coconut, realizing there is milk inside, smashing it and drinking it: not intuition, you inferred liquid from rationally thinking about the sound it made. Operating any user interface that requires a keyboard or mouse: not intuitive by any stretch of the definition. Somewhere, someone taught you how these things worked. So sorry, your cool new interface that passed usability testing with flying colors is not intuitive.

Your user interface might be familiar, and this is good because it reduces the learning curve of users who have already been trained in something similar.

Or your user interface might be easily habituated, and this is good because repeated use will increase the user's productivity and operation time.

But ease of use and speed of learning are certainly not the same thing as intuitiveness. As Jef Raskin points out in "The Humane Interface", "The mouse is very easy to learn... [It's] fast and easy, but it is neither intuitive or natural. No artifact is."

What you can do is make an interface self-teaching, meaning the user can always find proper explanations and instructions when they are needed.

So from here on, I pledge to stop saying "intuitive" when it comes to interfaces. Instead I'll stick with "familiar", "self-teaching", and "easily habituated".

AND WHILE WE'RE ON THE TOPIC... I often hear people say their co-workers are idiots. The word idiot traditionally, specifically describes people with an IQ of 0-25. It is one step below imbecile (26-50), and two below moron (51-70). Here is a handy table for comparison:

Term	IQ Range
Moron	51-70
Imbecile	26-50
Idiot	0-25

True idiots are as rare as the true genius. While it might be possible, however unlikely, that you work with a bunch of morons, it is almost unthinkably improbable that you work with a bunch of idiots. So from here on, I pledge to stop calling my co-workers of years past idiots, and instead will upgrade them to mere morons (I would never call any present co-workers either of course!).

Sadly, the work of Henry Goddard, who created this colorful scale of feeble-mindedness, is now politically incorrect, and since the turn of the century state governments have been moving to replace these terms with "mentally incompetent". This is discouraging for lovers of language, but at least we can take heart in the knowledge that at least 5 states do not allow idiots to vote. Surprisingly, none of those states voted Republican in the 2004 presidential election. Funny, I'd of thought it'd be the other way around.