Rules, rules. TDD rules, Beck Rules, OK. And a cat.

This morning it fell to me to feed the cat. My dear wife leaves a clean cat dish, cat serving spoons, and, if it’s time for a new can of food, a can of food on the counter. There was no can, so I knew that there should be an open can in the fridge, but I couldn’t find it.

Finally, I asked, and she showed me that it was behind the milk, on the grounds that I’d find it while making my morning chai. What she didn’t know was that first I feed the cat, to bring down the noise level, and only then make my chai.

At the end of the transaction, I said that it’s best to leave the can, with its bright red lid, where even a fool could find it. She said “You’re not a fool.” I replied “Oh yes, I am. I’m very smart, but I’m a fool.”

And I am a fool. I suppose most of us are. Just me? Well, OK. Anyway, as I program, I know most of the rules, and I break them, frequently, to my disadvantage. Surely that is foolish.

TDD Rules

The TDD rules are pretty simple: Red, Green, Refactor. And yet, in the Codea stats program, I have done essentially no refactoring. We’re advised by those rules to take every opportunity when the tests are green to improve the code.

Beck Rules

Kent Beck’s four rules of simple design are also pretty simple. The code is simple to the extent that, in priority order:

  1. It runs all the tests.
  2. It contains no duplication.
  3. It expresses all the programmer’s design ideas.
  4. It minimizes entities (functions, classes, methods …)

(Scripture varies on the order of rules 2 and 3. I prefer this order, because duplication is easy to spot and if the rule ever happened to conflict with rule 3, it would be very interesting, worthy of thought.)

Rulebreaker

If I’ve paid much attention to these rules in Codea Stats, I don’t recall it. I certainly haven’t paid much. So, today, let’s refactor, with an eye to Beck’s rules. (The TDD rules are his as well, by the way. The man is everywhere.)

Let’s Refactor

Normally I advise only refactoring “old” code when we have a story that passes through it. The code we’re not working with isn’t bothering anyone much, and this guideline helps us focus on the customer, and defers work until it is really needed.

Of course, sometimes, we just can’t resist. We can chalk it up to practice and learning. That’s what we’re at today. No story, just improving the code.

I’ll begin with a quick scan for targets. I do have some already in mind, that tripling of Collection and Description, but there might be other worthy areas. Some of them might even be easier to do. I like to start with something easy to warm up my mind, since I couldn’t even find the cat food.

I found a hookup test:

        _:test("HOOKUP", function()
            _:expect("Bar", "testing fooness").is("Bar")
        end)

Those should just be deleted. They are there to assure me that CodeaUnit has found the tests. Once they fail, there’s not much point fixing them,. Removed.

Commit: removed hookup test. I’ll try to commit a lot.

I found another one, removed it, committed again.

We have, as I mentioned, two sets of three very similar classes:

OOO! I found a bug:

function FunctionCollection:count()
    return 1
end

That should be:

function FunctionCollection:count()
    return #self.functionDefinitions
end

We should write a test for that, since it was obviously broken.

Sometimes, as we look for code improvement, we discover things like this. I could just commit the fix: I’m sure it’s right. But let’s have a test to go with it.

Oh look! The test we have is wrong!

        _:test("ProjectDescription named functions", function()
            local desc = ProjectDescription("SampleStatsProject")
            local functions = desc:namedFunctions()
            _:expect(functions:count()).is(1)
            local rep = functions:fullReport()
            _:expect(rep).is("setup()\ndraw()\nfind3Args(arg1, arg2, arg3)\n")
        end)

It should now break. Let’s see if it does.

1: ProjectDescription named functions  -- Actual: 3, Expected: 1

Life is good. Fix test, commit: fixed function count test and defect.

That was careless. I had a good chance of noticing that when I was surprised by thesetup and draw showing up. But I missed it. I’m glad we stopped by.

Triplication

But we were talking about the tripled classes that look rather similar.

FunctionCollection = class()

function FunctionCollection:init(funcTable)
    self.functionDefinitions = funcTable or {}
end

function FunctionCollection:add(anItem)
    table.insert(self.functionDefinitions, anItem)
end

function FunctionCollection:count()
    return #self.functionDefinitions
end

function FunctionCollection:fullReport()
    local result = ""
    for _i,func in ipairs(self.functionDefinitions) do
        result = result..func:fullReport()
    end
    return result
end


MethodCollection = class()

function MethodCollection:init(methodTable)
    self.methodDefinitions = methodTable or {}
end

function MethodCollection:add(anItem)
    table.insert(self.methodDefinitions, anItem)
end

function MethodCollection:count()
    return #self.methodDefinitions
end

function MethodCollection:countForClass(className)
    return self:methodsForClass(className):count()
end

function MethodCollection:fullReport()
    local tab = "        "
    local result = ""
    for _i,meth in ipairs(self.methodDefinitions) do
        result = result..tab..meth:fullReport()
    end
    return result
end

function MethodCollection:methodsForClass(className)
    local result = {}
    for _i,meth in ipairs(self.methodDefinitions) do
        if not className or meth:inClass(className) then
            table.insert(result, meth)
        end
    end
    return MethodCollection(result)
end

function MethodCollection:report()
    local tab = "    "
    local report = ""
    for _i, methodDef in ipairs(self.methodDefinitions) do
        report = report..tab..methodDef:report().."\n"
    end
    return report
end

ClassCollection = class()

function ClassCollection:init(classTable)
    self.classDefinitions = classTable or {}
end

function ClassCollection:add(anItem)
   table.insert(self.classDefinitions, anItem) 
end

function ClassCollection:count()
    return #self.classDefinitions
end

function ClassCollection:fullReport(methodCollection)
    local tab = "    "
    local result = ""
    for _i,classDef in ipairs(self.classDefinitions) do
        result = result..tab..classDef:fullReport(methodCollection)
    end
    return result
end

function ClassCollection:report()
    local tab = "    "
    local result = ""
    for _i,classDef in ipairs(self.classDefinitions) do
        result = result..tab..classDef:report().."\n"
    end
    return result
end

These classes all have a table. They all have add, and count, and fullReport. Two of them also have report, and the fullReport methods are similar but different. In particular, the ClassCollection has a parameter for its full report, and uses it.

The internal table, the add and the count are all clearly duplicated. The reporting, less so. What can we do to improve this situation?

One possibility is to create a new collector class, and defer to it, while maintaining separate reporting up above. This gains nothing, since each of our separate guys would still have their own add and count methods to defer down.

Another is to build a superclass that includes the table management and the adding and counting, and build three subclasses to do the reporting. We might find more duplication to remove. This approach has the drawback that we’d have concrete methods for adding and counting in the superclass, and some people will tell us that concrete methods in a superclass are a bad idea. Just now I’m not at all sure why that would be the case.

A third possibility is to build a general collection class with the adding and counting, which is created with some kind of pluggable behavior to do the reporting. We could create each collection with a reporter instance that does the work. I think that would wind up with the iteration being done in the collection class, and individual report lines being created in the reporters.

The third possibility seems most interesting to me, because we haven’t recently done anything like that. Let’s try it.

Hold My Chai

The question that interests me most here is how to do this change in tiny steps. If I could, I’d just let the existing tests serve, but I’m not sure whether I can build this thing in small steps without additional tests.

Let’s TDD it. The tests may be redundant when we’re done, but since this object isn’t like the others, we may find the tests serve as interesting documentation.

I make a new tab and this test:

        _:test("ItemCollection", function()
            _:expect(2).is(1)
        end)

Fails. Nice.

Now, what does this guy do? He adds items and counts them. Let’s start there.

        _:test("ItemCollection", function()
            local ic = ItemCollection()
            _:expect(ic:count()).is(0)
        end)

This fails:

1: ItemCollection -- ItemCollection:17: attempt to call a nil value (global 'ItemCollection')

Create the class.

ItemCollection = class()

Test will fail with no count.

1: ItemCollection -- ItemCollection:18: attempt to call a nil value (method 'count')

Implement count:

function ItemCollection:count()
    
end

Test will fail finding nil not zero.

1: ItemCollection  -- Actual: nil, Expected: 0

Return zero.

function ItemCollection:count()
    return 0
end

Test will run.

Ship it. Commit: initial ItemCollection passes its test.

Really? Yes, really. MMMSS, Many More Much Smaller Steps. System works. It’s “ready” as GeePaw Hill would say. Step ends. Whenever a step ends we can commit, and probably should.

Enhance the test:

        _:test("ItemCollection", function()
            local ic = ItemCollection()
            _:expect(ic:count()).is(0)
            ic:add(37)
            _:expect(ic:count()).is(1)
            ic:add(29)
            _:expect(ic:count()).is(2)
        end)

Bigger step this time. Of course I know what to do but instead, in the interest of rhythm, I run the test expecting no method add:

1: ItemCollection -- ItemCollection:19: attempt to call a nil value (method 'add')

Just to prove I can go in tiny steps if I want:

function ItemCollection:add(anItem)
    
end

Method largely useless. Test will fail with 0 expecting 1 and 2.

1: ItemCollection  -- OK
1: ItemCollection  -- Actual: 0, Expected: 1
1: ItemCollection  -- Actual: 0, Expected: 2

OK, let’s make it work, enough playing around.

ItemCollection = class()

function ItemCollection:init()
    self.collection = {}
end

function ItemCollection:add(anItem)
    table.insert(self.collection, anItem)
end

function ItemCollection:count()
    return #self.collection
end

I expect the test to run.

1: ItemCollection  -- OK
1: ItemCollection  -- OK
1: ItemCollection  -- OK

Sweet. Commit: ItemCollection collects items and counts them.

We could go home now, if it were 5 o’clock, secure in the knowledge that our day’s work is right there in HEAD. But it isn’t.

Next Phase

Our next phase should be reporting. Let me mention, though, that I am aware that my original collections seem to accept a starting table. I could look and see if that capability is used. I know that i replicated it blindly when I built the classes. Instead, we’ll ignore it. We can be sure that when we plug this new class into the operating part of the program, we’ll find the need if it’s there.

The plan is that we’ll instantiate ItemCollection with an instance of some class that implements, for example, “fullReport`. We’ll probably need to support a few more methods.

So I want a test that plugs in a reporter and produces a report. I decide to do it entirely with fake objects. I think it’ll be simpler to understand.

        _:test("ItemCollection fullReport", function()
            local ic = ItemCollection(FakeReporter())
            ic:add(37)
            ic:add(29)
            local rep = ic:fullReport()
            _:expect(rep).is("37\n29\n")
        end)

This will fail with no FakeReporter:

2: ItemCollection fullReport -- ItemCollection:26: attempt to call a nil value (global 'FakeReporter')

Build the class:

FakeReporter = class()

Now we should fail with no method fullReport on IC:

2: ItemCollection fullReport -- ItemCollection:29: attempt to call a nil value (method 'fullReport')

We need to do a bit of code now.

function ItemCollection:init(aReporter)
    self.reporter = aReporter
    self.collection = {}
end

function ItemCollection:fullReport()
    
end

I get this far and realize I’m a bit off track. Let’s look at a real “fullReport” method:

function FunctionCollection:fullReport()
    local result = ""
    for _i,func in ipairs(self.functionDefinitions) do
        result = result..func:fullReport()
    end
    return result
end

It’s almost true that all we do in our fullReport is accumulate the strings from the individual items. We are the collection. So my FakeReporter should be an item, not a collectionish thing, which was what I was thinking.

Redo the test a bit:

        _:test("ItemCollection fullReport", function()
            local ic = ItemCollection()
            ic:add(FakeItem(37))
            ic:add(FakeItem(29))
            local rep = ic:fullReport()
            _:expect(rep).is("37\n29\n")
        end)

I think I can make this run with this:

FakeItem = class()

function FakeItem:init(aNumber)
    self.num = aNumber
end

function FakeItem:fullReport()
    return self.num.."\n"
end

And with this:

function ItemCollection:fullReport()
    result = ""
    for _i,item in ipairs(self.collection) do
        result = result..item:fullReport()
    end
    return result
end

Run the test.

2: ItemCollection fullReport  -- OK

Now we do need to remember the almost. The real examples include headings like “Classes”, and some of them include a tab. We may still need to pass in some kind of information about that when we create our collection. We’ll see.

Tests run. Commit: ItemCollection defers fullReport to its items.

We’re on a fresh commit. Let’s try to make a real report run. I’d like to use one of the existing tests. Let’s look around to see if there’s an easy one.

When I look at ProjectDescription …

function ProjectDescription:init(projectName)
    self.projectName = projectName
    self.tabs = self:getProjectTabs(projectName)
    local classMatcher = MatchMaker:classDefinition()
    local methodMatcher = MatchMaker:methodDefinition()
    local namedFunctionMatcher = MatchMaker:namedFunctionDefinition()
    for _i,tab in ipairs(self.tabs) do
        local cont = tab:contents()
        cont = stripComments(cont)
        classMatcher:process(cont)
        methodMatcher:process(cont)
        namedFunctionMatcher:process(cont)
    end
    self.classCollection = classMatcher:collection()
    self.methodCollection = methodMatcher:collection()
    self.namedFunctionCollection = namedFunctionMatcher:collection()
end

I see that it is down to the MatchMaker to decide what the collection is:

function MatchMaker:classDefinition()
    return MatchMaker(ClassCollection, ClassDefinition, PatternDictionary:classDefinition())
end

function MatchMaker:methodDefinition()
    return MatchMaker(MethodCollection, MethodDefinition, PatternDictionary:methodDefinition())
end

function MatchMaker:namedFunctionDefinition()
    return MatchMaker(FunctionCollection, FunctionDefinition, PatternDictionary:namedFunctionDefinition())
end

So in principle, if MatchMaker passed in an ItemCollection instead of a (type)Collection, everything might just work. Which kind of collection only has fullReport? Only FunctionCollection. The other two have report, which is not yet implemented.

Let’s plug ItemCollection in and see what breaks.

function MatchMaker:namedFunctionDefinition()
    return MatchMaker(ItemCollection, FunctionDefinition, PatternDictionary:namedFunctionDefinition())
end

I actually rather expect this to work, but I’m prepared to be not quite right. The tests all pass. Now I’m wanting to be sure that we have hit the right tests, so I’ll break the report method:

function ItemCollection:fullReport()
    result = ""
    for _i,item in ipairs(self.collection) do
        result = result.."oops "..item:fullReport()
    end
    return result
end

Test again.

2: ItemCollection fullReport  -- Actual: oops 37
oops 29
, Expected: 37
29

3: MatchMaker finds named functions  -- Actual: oops find3Args(arg1, arg2, arg3)
, Expected: find3Args(arg1, arg2, arg3)

1: ProjectDescription named functions  -- Actual: oops setup()
oops draw()
oops find3Args(arg1, arg2, arg3)
, Expected: setup()
draw()
find3Args(arg1, arg2, arg3)

Super. We’re using the code and it’s working.

Remove the oops. Commit: MatchMaker uses ItemCollection for function collecting.

This is going nicely. We have now eliminated the necessity for FunctionCollection. Let’s look for references. I find none. Remove it. Test. Tests green. Commit: Removed unneeded FunctionCollection class.

What’s Next?

This has gone quite nicely so far. What’s next? I’m inclined to plug the ItemCollection into another MatchMaker and see what breaks. I pick MethodCollection because I think it might be easier.

function MatchMaker:methodDefinition()
    return MatchMaker(ItemCollection, MethodDefinition, PatternDictionary:methodDefinition())
end

Test see what breaks. Stuff will.

5: ProjectDescription fullReport -- TabTests:224: attempt to call a nil value (method 'methodsForClass')
2: MatchMaker finds methods method collection -- Actual: false, Expected: true
2: MatchMaker finds methods -- MatchMaker:45: attempt to call a nil value (method 'report')

Ah. The second one is easy:

The test is checking to see that we return a MethodCollection and of course we don’t. Remove that check.

            local isMethod = coll:is_a(MethodCollection)
            _:expect(isMethod, "method collection").is(true)

The third failure is the missing report method. We look to see what the existing report methods look like. I’m sure they just defer to the item’s report.

function MethodCollection:report()
    local tab = "    "
    local report = ""
    for _i, methodDef in ipairs(self.methodDefinitions) do
        report = report..tab..methodDef:report().."\n"
    end
    return report
end

function ClassCollection:report()
    local tab = "    "
    local result = ""
    for _i,classDef in ipairs(self.classDefinitions) do
        result = result..tab..classDef:report().."\n"
    end
    return result
end

This is handy, they both use the same tab. I won’t have to solve that problem yet.

function ItemCollection:report()
    local tab = "    "
    local result = ""
    for _i,item in ipairs(self.collection) do
        result = result..tab..item:report().."\n"
    end
    return result
end

I expect to have two of the tests fixed. Test and see:

5: ProjectDescription fullReport -- TabTests:224: attempt to call a nil value (method 'methodsForClass')
2: ProjectDescription methods -- TabTests:119: attempt to call a nil value (method 'methodsForClass')

Were there two of those before? Probably. What are these calls anyway?

function ProjectDescription:methods(className)
    return self.methodCollection:methodsForClass(className)
end

function ClassDefinition:fullReport(methodCollection)
    local result = self:report()
    local myMethods = methodCollection:methodsForClass(self.className)
    local count = myMethods:count()
    result = result.." ("..count..")\n"
    result = result..myMethods:fullReport()
    return result
end

Those are triggered here:

        _:test("ProjectDescription methods", function()
            local desc = ProjectDescription("SampleStatsProject")
            local methods = desc:methods()
            _:expect(methods:count()).is(5)
            local sampleCodeMethods = desc:methods("SampleCode")
            _:expect(sampleCodeMethods:count()).is(4)
        end)

Interesting

Now we have an interesting problem. Our generic collection of items is being asked to find items whose class is a provided, possibly missing, string.

It seems to me that while we could just put that method on ItemCollection, it seems a bit specific for a generic collection.

I think we need to do that, if only to see what it looks like, so that we can try to improve it.

Let’s try. I’m a bit concerned because it has been 15 minutes since a commit and this may get wonky. I’d like to have a place to revert to. But I’m not green.

Go for it. The original method is:

function MethodCollection:methodsForClass(className)
    local result = {}
    for _i,meth in ipairs(self.methodDefinitions) do
        if not className or meth:inClass(className) then
            table.insert(result, meth)
        end
    end
    return MethodCollection(result)
end

I’ll copy that over and then figure out how to make it workable.

function ItemCollection:methodsForClass(className)
    local result = {}
    for _i,meth in ipairs(self.methodDefinitions) do
        if not className or meth:inClass(className) then
            table.insert(result, meth)
        end
    end
    return ItemCollection(result)
end

Now we see why our collections accept a table. This is the case. Add that:

function ItemCollection:init(coll)
    self.collection = coll or {}
end

We still had the reporter stuff in there but I think it’s not used. Run the tests.

5: ProjectDescription fullReport -- attempt to index a nil value
2: ProjectDescription methods -- attempt to index a nil value

That could be more helpful, couldn’t it?

Some judicious enhancement to the test tells me that whatever happened … i bet I didn’t change my collection name, did I? …

function ItemCollection:methodsForClass(className)
    local result = {}
    for _i,meth in ipairs(self.collection) do
        if not className or meth:inClass(className) then
            table.insert(result, meth)
        end
    end
    return ItemCollection(result)
end

There’s a reason why copy-paste is not recommended, but I don’t remember what it is.

Test.

5: ProjectDescription fullReport  -- Actual: Classes

    SampleCode (4)
init(x)
draw()
touched(touch)
extraMethod(a,b,c)
    SubclassOfSample(SampleCode) (1)
init(ignored)
, Expected: Classes

    SampleCode (4)
        init(x)
        draw()
        touched(touch)
        extraMethod(a,b,c)
    SubclassOfSample(SampleCode) (1)
        init(ignored)

Excellent! Our problem here is that we are missing our tabs on the methods. That should be fixable.

function ItemCollection:fullReport()
    result = ""
    for _i,item in ipairs(self.collection) do
        result = result..item:fullReport()
    end
    return result
end

How about if we pass in the tab stop we want?

function ItemCollection:fullReport(tabString)
    local tab = tabString or ""
    local result = ""
    for _i,item in ipairs(self.collection) do
        result = result..tab..item:fullReport()
    end
    return result
end

Now when we do the methods report, let’s pass in 8 spaces.

Wait, I’m confused. The ProjectDescription has this:

function ProjectDescription:fullReport()
    report = "Classes\n\n"
    report = report..self.classCollection:fullReport(self.methodCollection)
    return report
end

function ClassCollection:fullReport(methodCollection)
    local tab = "    "
    local result = ""
    for _i,classDef in ipairs(self.classDefinitions) do
        result = result..tab..classDef:fullReport(methodCollection)
    end
    return result
end

function ClassDefinition:fullReport(methodCollection)
    local result = self:report()
    local myMethods = methodCollection:methodsForClass(self.className)
    local count = myMethods:count()
    result = result.." ("..count..")\n"
    result = result..myMethods:fullReport()
    return result
end

Ah. We can pass the tab in there. I think I need 8 spaces, but I could be wrong.

I do this, because I want to see the result clearly.

    result = result..myMethods:fullReport("++++++++")
5: ProjectDescription fullReport  -- Actual: Classes

    SampleCode (4)
++++++++init(x)
++++++++draw()
++++++++touched(touch)
++++++++extraMethod(a,b,c)
    SubclassOfSample(SampleCode) (1)
++++++++init(ignored)
, Expected: Classes

    SampleCode (4)
        init(x)
        draw()
        touched(touch)
        extraMethod(a,b,c)
    SubclassOfSample(SampleCode) (1)
        init(ignored)

I think that’s right. Make spaces.

Tests are green. One more thing, can we remove MethodCollection? Tests say yes.

Commit: Replaced MethodCollection with ItemCollection. Moderately odd method ‘methodsForClass` on ItemCollection. Too specific.

I think we’ll sum up. It’s noon, and I’ve been here since around 10 AM or a bit earlier.

Summary

We have a net reduction in code, by providing the capability of MethodCollection and FunctionCollection in the single class ItemCollection.

We’ve removed essentially all the duplication that existed between those two classes, moving it essentially as-is into the single new class.

And we mostly went in small steps. Counting article writing time, our commits were at 1008, 1010, 1017, 1045, 1052, 1111, 1125, 1127, and 1210. Two long ones in there, 28 minutes and 43 minutes, for the last one. Not bad at all, but not great, especially for that last one.

Perhaps there was a better way to do that, but I don’t see it. Anyway, 9 commits in a couple of hours isn’t all bad. Average of less than 14 minutes. Not bad for a fool.

Bottomlineish …

Removing duplication has certainly simplified the code. It’s easier to understand one class, ItemCollection, than two essentially identical classes. We do have an expression issue, in the the methodsForClass method seems a bit out of place. It kind of makes a bump in the mind. We’ll see if we can address that next time.

Tomorrow, we’ll move forward with this and see where we stand. We have a few more function types to do, and I think they might turn out to be tricky.

See you then, I hope!