Some refactoring, and some thoughts on a Twitter thread. And an odd surprise: the code says No.

The other day, I tweeted:

if someone thinks refactoring belongs on the backlog, they probably don’t understand refactoring, or backlogs, or both.

I don’t recall now who or what triggered this, but I was probably subtweeting something. I’ve written about this before, of course, here.

The gist is that once we notice that some committed code would benefit from refactoring, we should generally refactor it incrementally, as we pass through it, or encounter it, in the course of our everyday feature work. We should generally not refactor code that isn’t in our way, as it is wasteful to do so, and we should not attempt to “get permission” to refactor the code as we encounter it. We should not, in my view, attempt to get refactoring put on the backlog as a story. The benefits of refactoring are almost impossible to compare with the benefit of whatever feature our customer, product owner, boss, …, is looking for.

Some will argue that they can say “doing this refactoring will allow us to go faster”, to sell the idea. I’m quite sure you could sell the idea, but the problem is that we don’t know how much faster we’ll go, and when we allocate a big chunk of time for a “big” refactoring effort, we may never get the time back.

Anyway, that’s my advice, take it or leave it.

Inevitable Remarks

There were some inevitable responses to the tweet. Here are two, rephrased for my purposes:

Delaying Refactoring Can Be Good
The idea is that refactoring slows us down, so that we can make a short-term decision to deliver something by deferring refactoring until later. An associated notion is that somehow this will allow us to assign a priority to refactoring and therefore schedule it in the backlog.

This notion is mistaken in two regards. First, despite the claim, I have never, ever, seen a business-side person who could make sensible priority decisions between features and what they’ll consider to be “code niceties”. To be frank, I can play both business and technical hands rather well, and I don’t think I can make those decisions either. The two are just not comparable.

More important, the major premise is mistaken. Recall that the tweet says they “don’t understand refactoring”. Refactoring, done incrementally, does not slow us down. Oh, sure, if I refactor for 20 minutes this morning, that’s 20 minutes of not writing some feature. But I’d wager that the folks arguing that refactoring slows them down are looking at features that take them days or weeks to implement, and they’re thinking about days to weeks of refactoring. I’m sorry to pull the old No True Scotsman out of the hat, but if you’re doing that, you’re doing it wrong.

Refactoring, done properly, maintains a steady pace of development, and is, over the middle and long term, the fastest way of delivering what we’re asked to deliver. If you’re as sensitive as my friends and I are, you can tell tomorrow that yesterday’s code needed more refactoring. It slows you down immediately, not days or weeks out.

And I’m sorry, but if this is not your experience, the first thing I’d want to do is to sit down with you and help you code and refactor. I’d bet my day’s pay, double or nothing, that we’d find a way to proceed that gives the lie to the notion that refactoring slows you down.

Of course, people are free to believe that refactoring slows them down, they’re free to do it in such a way that it does slow them down, and they will surely be correct. There are ways of refactoring that will slow you down. We in the trade call those “bad ways” of refactoring.

Teams Need a “Fix the Mess” Rule
This comment arose, and I guess I can’t disagree. If a team doesn’t have a focus on fixing the messes they make–and we all make them–they will inevitably slow down more and more. I wouldn’t call it a rule. I might call it “caring”, or “giving a damn”, or even “programming like a professional”.

If a plumber came to my house and put in pipes that sloped the wrong way, plugged leaks with rags, and left all my faucets leaking, I would feel justified in saying that they were not a good plumber. The results tell me that. If a team isn’t cleaning up the messes that they inevitably make, well, I feel justified in saying that they aren’t very good programmers.

The plumber could learn. The programmers could learn. They could start by just giving a damn, and not stopping work until they sorted out how to do it without rags stopping the leaks.

So, as one expects in these days, I maintain my position, and for all I know, the other people in the conversation maintained theirs. I hope that some listeners were motivated to think about the ideas, to learn a bit, and to see if they could get some of this “good refactoring” we talk about.

Now let’s do some.

Is This Even Good Refactoring?

Well, on the one hand, I certainly hope it’s good. That is, I hope that the code is discernibly better when we’re done than when we started. On the other hand, I would not argue that the refactoring we’ve done recently will speed up progress on features, because we’re nearly done with features.

The code we’re refactoring wasn’t really bothering anyone. It is highly duplicated, having essentially been cut and pasted into being. I think it slowed me down a bit, if only because I had to implement half a dozen methods each time, where with the refactored code it might only be a method or two. But we’re nearly done with this program.

We’re doing this for practice refactoring, not quite the same thing as refactoring as we go in a real project.

There is an important lesson to learn, should you decide to do some practice refactoring, or even to refactor a bit in your real program. The lesson is that we can usually do our refactoring in very small steps, spaced out over time. We don’t have to clean up the entire campground: we just have to leave it a bit better than we found it.

Anyway, let’s do it.

Where Were We?

We’re in the process of combining the ClassCollection, MethodCollection and FunctionCollection classes into a single ItemCollection class. This will reduce the code for that area to something between one third and one half of what it originally was. And it’ll reduce the general complexity of the program.

We’ve consolidated MethodCollection and FunctionCollection already. Today, we’ll see about ClassCollection.

I’m at a clean commit. Let’s just plug in the ItemCollection where we use a ClassCollection and see what breaks. Here’s the only place it’s used:

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

We replace with:

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

And we test. I think there’s a test checking to see if a ClassCollection comes back. And I won’t be surprised if something else breaks. Our purpose here is to get a sense of how tricky this change will be.

1: MatchMaker finds classes class collection -- Actual: false, Expected: true

Not very informative, but it’s this:

        _:test("MatchMaker finds classes", function()
            local mm = MatchMaker:classDefinition()
            local tab = readProjectTab("SampleStatsProject:SampleCode")
            tab = stripComments(tab)
            mm:process(tab)
            local coll = mm:collection()
            local isClass = coll:is_a(ClassCollection)
            _:expect(isClass, "class collection").is(true)
            _:expect(coll:count()).is(2)
            local rep = coll:report()
            _:expect(rep).is("    SampleCode\n    SubclassOfSample(SampleCode)\n")
            local tinyTab = "Argle = class()"
            mm:process(tinyTab)
            _:expect(coll:count()).is(3)
            _:expect(coll:report()).is("    SampleCode\n    SubclassOfSample(SampleCode)\n    Argle\n")
        end)

It’s that bit with is_a. We can just remove that check.

        _:test("MatchMaker finds classes", function()
            local mm = MatchMaker:classDefinition()
            local tab = readProjectTab("SampleStatsProject:SampleCode")
            tab = stripComments(tab)
            mm:process(tab)
            local coll = mm:collection()
            _:expect(coll:count()).is(2)
            local rep = coll:report()
            _:expect(rep).is("    SampleCode\n    SubclassOfSample(SampleCode)\n")
            local tinyTab = "Argle = class()"
            mm:process(tinyTab)
            _:expect(coll:count()).is(3)
            _:expect(coll:report()).is("    SampleCode\n    SubclassOfSample(SampleCode)\n    Argle\n")
        end)

5: ProjectDescription fullReport -- TabTests:178: attempt to index a nil value (local 'methodCollection')

Hm, that’s here:

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

Somehow, the methodCollection parameter is nil. Quick checks aren’t showing me what’s happening. Let’s follow the code. Here’s the test:

        _:test("ProjectDescription fullReport", function()
            local desiredReport = [[
Classes

    SampleCode (4)
        init(x)
        draw()
        touched(touch)
        extraMethod(a,b,c)
    SubclassOfSample(SampleCode) (1)
        init(ignored)
]]
            local report = ProjectDescription("SampleStatsProject"):fullReport()
            _:expect(report).is(desiredReport)
        end)

‘ProjectDescription:fullReport’ goes like this:

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

Well, that would seem to say we don’t have a method collection. I’ll put in an assert there to check it. It doesn’t assert, so the error must be further down.

What was ‘ClassCollection:fullReport, and what is the case for ItemCollection`?

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 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

Hm. This isn’t going to be trivial. Revert and go another way.

My plan is to reduce ClassCollection and make it a subclass of ItemCollection. I hope not to leave it that way, but we’ll see.

ClassCollection = class(ItemCollection)

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

That came down to removing the init, add, and count methods. Tests should run. They don’t:

1: MatchMaker finds classes -- MatchMaker:85: attempt to call a nil value (method 'add')

I thought I had moved ItemCollection’s tab ahead of ClassCollection (so it can inherit) but I guess my finger slipped. Now I get these errors:

4: ProjectDescription classReport -- attempt to index a nil value
5: ProjectDescription fullReport -- attempt to index a nil value

That’s because I didn’t change the member variable references.

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

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

There are times, I admit, when I could wish for a more restrictive compiler. Don’t quote me or at me.

Tests pass. Commit: ClassCollection inherits from ItemCollection, removed three methods.

It seems to me that ItemCollection:report is the same as ClassCollection. Remove the latter, test.

Tests run. Commit: remove redundant ClassCollection:report method.

We’ve improved things, though not as far as we’d like. And we’re leery of the use of inheritance here, though I freely grant that I don’t see the objection folks would raise.

Let’s compare those two fullReport methods, see what we can learn.

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

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

I hate that tabString thing. I am tempted to divert to improve that somehow.

Nonetheless, what we see here is that the class report really is different from the others, in that it produces a sub-report, the list of its methods, and the other reports do not do that.

I wonder if we couldn’t move the sub-report up to the higher level. Why is the class collection worrying about other folks?

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

Ah, yes. Since the looping is done in the collection, the searching for the related methods must be done there as well.

However, while a collection can legitimately be asked for an iterator, it doesn’t really make sense for it to know enough about its members. I think the code is telling us that the report looping should be done at the project level. The individual item reporting is in the items. That makes sense.

Oh Wow

This is interesting. (I claim.) If we were to move group reporting up to the ProjectDescription, then the ItemCollection would consist of nothing but add, count, and iterate. Lua tables can do that.

I think we can make a case that I’ve built way more structure than I needed to. The individual items make sense, because they understand different numbers of captures and different formatting. But the collections … we were already reducing three down to one. I think they reduce down to zero, if we wish.

We might not wish. We do have a need to do that query method:

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

That makes too many assumptions. It’s on our list to look at that.

This needs some consideration. Let’s start thinking about it now, but I’m not sure we’ll decide what to do today, nor act on a decision if we make one.

Options

I see two main options. We can call it done and proceed with additional features for the program, or we can move reporting up into ProjectDescription and factor down to a single collection class. If we choose the latter path, we have the issue of the method-selecting function, which is a bit of a lump on the side of what should be a simple collection class.

If this were a real project, I’d say “call it done”, confident that what we have is simple enough for now, and if we come up with new needs or better ideas, we can deal with them then.

Since my point with these articles is to learn, and to perhaps inspire others to learn, I am inclined to push further on getting the shape of the program right.

I think it’s clearly not right now. The fact that two of three collections are trivial and one isn’t is a bit odd. Not enough to be wrong, but odd. The excessively concrete selection method on a generic collection is another odd thing. Maybe there should be a select function but if there is, it shouldn’t know about things like class names, it should take some more general syntax.

It seemed to make sense to push function down into the collections, but I am now questioning that idea. I was trying to lean away from using plain tables, but I think it has led us into other complexity and oddness.

So we’ll go with Plan B, move reporting back up into ProjectDescription, or down into the individual item definitions, with nothing intelligent in the collections.

Let’s start today. I think I can work a bit more, despite some distractions.

Class Reporting at Top

Here’s our reporting code now:

function ProjectDescription:fullReport()
    local 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.collection) do
        result = result..tab..classDef:fullReport(methodCollection)
    end
    return result
end

And the corresponding:

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

Hm. If we were to remove the ClassCollection method, but we were to pass the methodCollection as a second parameter, and pass it down … let’s try that.

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

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

Let’s see what this does. I don’t think we’ll keep it but we might learn a bit. We only get one error! This is good news:

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)

Hm. Looks like I can try the four spaces and things might work.

The tests all run!

Remove ClassCollection and the reference to it.

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

Test. Tests Run. Commit: Removed ClassCollection. All MatchMakers use ItemCollection.

Isn’t That Interesting!!

So, without pushing reporting upward, we now have a generic fullReport method in ItemCollection that defers down to the various Definitions, and the ClassDefinition knows how to get the methods out of a provided method collection.

Let’s look at the two reporting methods in ItemCollection:

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

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

We can make these two more similar. Let’s do so by passing in the same parameters.

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

At this point, I think this will just work as is. Test. However, if I make the tabString default the empty string, I expect a fail.

Yes. I’ll change all the calls to it to include four spaces. Tests work.

Now those two methods are almost identical:

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

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

At this point, I went further, messed up, reverted, and redid this change. I should have done this:

Commit: make ItemCollection report and fullReport look more similar.

You’re probably wondering why I did that. I did that hoping that I can make the two methods totally the same, except for the call to report or fullReport. If I can accomplish that, I can possibly consolidate the two into one. I think that would be a good thing.

I’ll make one attempt further, and if it doesn’t work, close for the day.

The report has a newLine at the end of its work, fullReport does not. Let’s add one to fullReport and see what breaks.

1: ProjectDescription named functions  -- Actual: setup()

draw()

find3Args(arg1, arg2, arg3)

, Expected: setup()
draw()
find3Args(arg1, arg2, arg3)

There are other breakages but we see this double spacing. The method probably returns one …

function MethodDefinition:fullReport()
    return self.methodName.."("..(self.args or "")..")\n"
end

function MethodDefinition:report()
    return self.className..":"..self.methodName.."("..(self.args or "")..")"
end

We should do this one way or the other. We have extra returns, but it sure does seem that each report should include its own newline. So I think full is right and report is wrong. That’s going to make me change the other guys in the other direction.

function MethodDefinition:fullReport()
    return self.methodName.."("..(self.args or "")..")\n"
end

function MethodDefinition:report()
    return self.className..":"..self.methodName.."("..(self.args or "")..")\n"
end

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

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

Test.

4: ProjectDescription classReport  -- Actual: Classes

    SampleCode    SubclassOfSample(SampleCode), Expected: Classes

    SampleCode
    SubclassOfSample(SampleCode)

Now I have no return at all after the class names.

Let’s see …

function ClassDefinition:report()
    local result = self.className
    if self.superclassName and self.superclassName ~= "" then
        result = result.."("..self.superclassName..")"
    end
    return result
end

Add one at the very end:

function ClassDefinition:report()
    local result = self.className
    if self.superclassName and self.superclassName ~= "" then
        result = result.."("..self.superclassName..")"
    end
    return result.."\n"
end

Test. Just one fail:

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)

Missing return after the full report for a class, that is, after its methods come out.

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

I think a return added there at the end …

    return result.."\n"

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)

Oh, did I read the previous one wrongly? We don’t want spacing between them. Remove that return, rerun.

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)

Oh. The return after the class name, where the method count is. That’s a bit more tricky. And I’m out of time. Revert again.

Let’s sum up.

Summary

The bear bit me a couple of times, but we did manage to get rid of the ClassCollection, so that now everyone uses ItemCollection.

We have two report functions that are very similar. The difference between them, really, is that we built the simple reports as a matter of convenience for testing. As yet, we have no story for a report of classes without methods, and if we did we could implement it differently.

Rather than making them the same, we might do well to use only fullReport, adjusting tests as needed to allow that to work. That loses a bit of “history” from the tests, but that’s probably OK. We’ve already lost a lot when we got rid of the extra collections. So a bit of revision to the tests might not be a good thing.

Fascinating …

To me, the fascinating bit is that I was feeling zealous about not using built-in collections, so I made custom collections right out of the box, and the code is rather clearly telling us now that there’s probably no need for more than one of them, and perhaps not even that one.

The notion of the Composite Pattern comes to mind here. Our reports are all small tree structures, and our code is treating everything pretty uniformly. We send report to our collections and they send report to their elements. If we had a collection of our collections at the top, we could (almost?) send report to that collection, which would send report to the sub-collections, which would send report to their elements and it would all unwind and be good.

Thinking about that, it seems that the glitch comes with the ClassDefinition, which, if we were doing Composite Pattern, would probably need to come in two flavors, one that acts like a collection of methods, and one that doesn’t.

Another possibility would be to build a tree that had a leaf for the class itself, and then a collection of methods (for that class). Instead of the class definition dealing with the methods, spanning the tree would “just” do it.

I’m not at all sure we want to do that. But it’s an interesting angle.

In Any Case …

There’s a kind of bumpiness or irregularity in here, in all the forms we’ve tried. There’s odd formatting for spacing and newlines threaded into the data objects, and there’s that strange method on the collection that knows how to deal with a particular type of element of a provided input collection.

I think the design is better now, since we have only one kind of collection instead of three that were essentially identical.

But it’s not “right”. Should we make it “right”, or leave it at good enough?

I think the practical answer is “good enough”, and the interesting learning answer is to try to move toward right. In a real app, I’d say good enough for now. In this one … we’ll see what I do.

Stop by tomorrow and we’ll both find out.