Refactoring Isn't Rework
Contents:
Introduction
There has recently been a bit of discussion on the Yahoo group about how much design up front is enough. In the course of that, I declared that refactoring isn’t rework, to which Alistair Cockburn retorted: “er… refactoring is exactly rework. If ever there was a thing that was rework, refactoring is it.” Alistair is formally correct: “rework”, in the dictionary, just means revision, and certainly refactoring, indeed any kind of incremental development, involves revision. But to many folks – including a lot of software people and managers – rework is considered bad, and to be avoided. I’m using the word in that sense here, not Alistair’s more benign definition.
Rework is, well, doing the work over again. While it is possible to rework a program using refactoring techniques, software development via simple design with refactoring really isn’t much like rework.
The short explanation is that programming isn’t typing, a notion that I borrowed for this purpose from Martin Fowler. A longer explanation is this article.
A Simple Problem
We’ll implement a small program, test-first, with simple design and refactoring. The problem will be to implement a stack, that can push and pop. Our mission is to watch the unfolding development to see how much rework there is in the building of the program. To make it interesting, I’ll implement the stack in a way that I’ve never used before.
Now, of course, I’ve thought about this article, and I know a fair amount about stacks, since I was around when they were invented. What I haven’t done is figure out just how I’m going to do this version, and I’ll try hard not to think about the design before I get there. So come along with me, and don’t think about a pink elephant at any time in your reading of this article.
First Test
I’m going to do this in Ruby, mostly because it’s easy and won’t take much time. I’m hoping to get this article done before I have to check out of the hotel. My first test today is just there to be sure that I have the Ruby project set up correctly. It looks like this:
class TestStack < TestCase def testHookup assert 1==1 end end
First Pop
The first real test will be one that creates a stack and pops it. I define that an empty stack, popped, returns nil. The test is:
def testPopEmpty s = Stack.new assert_equal(nil, s.pop) end
It doesn’t compile because Stack isn’t defined, so I write the class:
class Stack end
Now the test doesn’t run, because pop isn’t defined. So I’ll define it to return nil:
def pop return nil end
I expect the test to run … and it does.
Push then Pop
That wasn’t much of a pop. I’ll write a new test that puts something on the stack and gets it back.
def testPushPop s = Stack.new s.push("a") assert_equal("a", s.pop) end
I don’t expect this to run, because push isn’t defined … and it doesn’t. So I’ll define push to save the input, and fix pop to return it. I’m going to do something really dumb here, but I swear it’s just for pedagogical purposes.
def pop return @saved end def push (x) @saved = x end
The tests all run fine. Ruby warns me, however, that @saved isn’t initialized. That’s coming from the first pop test. I decide not to worry about it right now, because the test ran.
Now then. To make the test work, I wrote that whole new method “push”, but I also edited the return line from pop. It used to say return nil, now it says return @saved.
Was that rework? I claim that it was not, because programming isn’t typing. When I first wrote that method, I didn’t design much of anything, I just thought “the test wants nil, give it nil”. In this second not very bright version, I did a little new designing. I thought: “I have to save the push input and return it later”. So the work was new work, not rework. The typing was rework: I replaced three of the old characters with six new ones. (Right now there are 299 characters in the program, so I’ve “reworked” about one percent of it.)
Push Push Pop Pop
OK, it’s time to quit messing around. Now for a hard test. We’ll push two things, then pop twice and make sure that we get the things back in the right (reverse) order.
def testPushTwice s = Stack.new s.push(1) s.push(2) assert_equal(2,s.pop) assert_equal(1,s.pop) assert_equal(nil,s.pop) end
I decided to go whole hog here and make sure that after the stack is empty, it returns nil. If I had thought to do that test in the testPush method, it would have failed, and it would have caused the program’s evolution to move in a different direction. But I didn’t think of it and we’re here now. We expect the test to fail (I expect the pop of 1 to fail, to be specific) … and it does.
OK, now I have to do some work. I have decided that I’ll implement this stack using a linked list. So I’ll just state my intention in the code. I’ll have to write a few lines here, so hold on:
def push (x) @saved = ListElement.new(x,@saved) end
The idea is that I’ll create a new ListElement (this class doesn’t exist yet) that will contain X, and whatever is already in @saved, and put the list element into @saved. Now to pop the stack, we have to return the item that’s in the first element in @saved, and set @saved to the next element. That looks like this … no, wait. I think I’ll run the test now, and get the error about ListElement and fix that first. I’m going to do that because it’s a bit more incremental, I won’t have so much to do in one go.
class ListElement def initialize(object, link) @object = object @link = link end end
OK, that compiles, but a couple of tests fail (as we’d expect). The errors both complain that instead of the expected answer, the test sees a ListElement as the result of a pop. No surprise: the pop method is just answering @saved. We need to change the pop method to return the object in the ListElement in @saved, and to put the link of the ListElement into @saved. I’ll code that. It’s going to be a little tricky, but I think I’m up to it.
def pop result = @saved.object; @saved = @saved.link return result end
I see, as you do, that I haven’t implemented the object and link methods on LinkElement. I’ll run the test to see that I get that error. (I am also expecting another error, that will occur when @saved is nil.)
Sure enough, I get undefined method “object”. I’ll implement those:
class ListElement def initialize(object, link) @object = object @link = link end attr_reader :object attr_reader :link end
The attr_reader statements define read accessors for the class. That’s all we need to move on to the next error. As I expected, the next error says that nil does not respond to “object”. No surprise. We need to do something about coming in to a pop when @saved is nil. And I’m still getting that warning about @saved not being initialized. Let’s fix the first problem first. That’ll take just one line added to pop:
def pop return nil if @saved == nil result = @saved.object; @saved = @saved.link return result end
We check @saved for nil and if it is, we answer nil, because that means the stack is empty. I’ll run the tests … and they all run. Now for that pesky warning. I’ll just fix that by adding an initialize method that nils @saved:
def initialize @saved = nil end
And the tests all run.
Final Answer?
We’ve gone from zero to a stack implementation, in just a little while. (Not quite as little as I’d hoped: I’m finishing this up at 30,000 feet on the way home from Atlanta.) The stack was built test-first, using simple design and refactoring. Here’s the whole program and its tests:
class TestStack < TestCase def testHookup assert 1==1 end def testPopEmpty s = Stack.new assert_equal(nil, s.pop) end def testPushPop s = Stack.new s.push("a") assert_equal("a", s.pop) end def testPushTwice s = Stack.new s.push(1) s.push(2) assert_equal(2,s.pop) assert_equal(1,s.pop) assert_equal(nil,s.pop) end end class Stack def initialize @saved = nil end def pop return nil if @saved == nil result = @saved.object; @saved = @saved.link return result end def push (x) @saved = ListElement.new(x,@saved) end end class ListElement def initialize(object, link) @object = object @link = link end attr_reader :object attr_reader :link end
Rework, or New Work?
Was there rework? Darn little. Almost all the code changes were of the nature of additions, with just a few characters replaced here and there. And there wasn’t much rethinking, either. During the initial “easy” tests, I laid in the overall structure of the system: the class and the method definitions, almost empty, with just a line in each one to make my tests run. Then there was a final sprint, itself done in little phases aided by the tests, where I implemented the ListElement, used it, enhanced it.
I was here, and I can tell you that it didn’t feel like rework, it felt like adding, adding, adding. Sure, there were a few lines replaced, but by far the bulk of the typing, and the thinking, was new, not rework.
Rework on painting your living room is when you have to repaint a whole wall, not when you touch up a couple of spots. Rework on your term paper is when you have to write most of it over again, not when you run the spell checker and fix a few typos and grammos. And rework on your software is when you have to rip out a whole bunch of code and do it over, not when you add new things and move a few things around.
Refactoring isn’t rework. It is revision, but it isn’t doing the work over.