Completing a refactoring makes a feature easier. Unless I’m wrong, but I’m not.

Pretty brave of me to call a shot like that, but in fat, after this query line refactoring is complete, I want to put back the feature that allows the base of the dialog line to follow a moving dungeon creature. I had thought we could do without that, in the aid of simplicity, but when I saw the on-screen effect, I realized it was better the old way. I believe, based on limited thinking—the only kind I really have—that it should be easy to change. I already have a couple of ideas for how to do it, so I’m sure to learn something.

Step one, however, is to cause all the objects who can use the base version of query do use it. Let’s get to it.

Using the Base Implementation

There are a few reasons for doing this change at all. The original implementations of query were all very simple and somewhat similar. Here’s a remaining example:

function Key:query()
    Bus:informDotted("Could I possibly be ... a key???", self:graphicCenter())
end

They are all more or less like that, as we’ll see as we check each one and decide what to do. Originally the rule was: do Bus:informDotted with whatever your query message is. Then, when we had the dialog line idea, the rule was extended: optionally include an object that can return your screen location, from which we’ll draw the line.

A few changes later, we switched to just passing the location in on the informDotted, which simplified the code but I don’t like it.

Anyway, the code for query is quite similar in each object that responds to it, with small differences depending on how they get their message and/or their location. And when we see duplication, we see an opportunity to improve the code. In this case, I interpret the opportunity as a chance to refactor to this situation, as I wrote yesterday:

  1. All or most DungeonObjects inherit the query implementation from DungeonObject;
  2. That top-level method calls self:getQueryMessage() and self:getQueryLocation(), or words to that effect;
  3. Individual classes implement those methods according to their likes. Most of them do not implement getQueryLocation which defaults to self:graphicCenter, again in DungeonObject.

In this refactoring, we remove the duplication of Bus:informDotted from many methods and leave it in one. And we identify the other two activities and give them names. In doing this refactoring we make the code more expressive, and we’ve arranged things so that if the way we do a query changes, our code changes will likely be more consolidated, and thus easier to make.

Mind you, I’m not doing this because I think someday I’m going to save a fortune by not having to seek out all the informDotted calls. I’m doing it because the code is, in my view, better this way, and since it’s easy to do, I do it. Over time, I find that this pays off in making everything a bit easier to understand, and often saves a bit of work, and associated confusion.

But I do not do this because it will pay off in the future. I really do it because it pays off today. When I improve the code, it feels good. I feel good. Job well done kind of thing. As Hill calls it: Geek Joy.

I don’t like the word “geek” personally. It seems pejorative—which, I believe, is why Hill is co-opting it. I’m not saying I ever had a really nasty summer job in a traveling carnival as “See! The skinny boy bites the heads off chickens!”. I’m just saying that I don’t care for the word. But I digress.

Since we’re looking at it, let’s cause the Key to use our new approach. We just change the old query method to this:

function Key:getQueryMessage()
    return "Could I possibly be ... a key???"
end

And voila! We’ve done Key. Moving to the next easy one:

function Decor:query()
    Bus:informDotted(self:queryMessage(self.kind), self:graphicCenter())
end

Same trick:

function Decor:getQueryMessage()
    return self:queryMessage(self.kind)
end

I edit the query methods to get the new getQueryMessage ones, which helps me ensure that I don’t leave any overrides lying around. Moving to the next easiest one … Oh, wait. These are trivial, but I really ought to test and commit. Commit: Key and Decor use inherited query.

Now:

function Lever:query()
    Bus:informDotted("At first glance, this appears to be a lever.\nDo you think it does anything?", self:graphicCenter())
end

Becomes, of course:

function Lever:getQueryMessage()
    return "At first glance, this appears to be a lever.\nDo you think it does anything?"
end

Test, commit: Lever uses inherited query.

Next:

function NPC:query()
    self.fsm:event("query")
end

I think this one needs to be left alone. It has its own scheme and it is somewhat complicated.

We’re left with Loot and Monster, I think. Monster is easy:

function Monster:query()
    Bus:informDotted(self.behavior.query(self), self:graphicCenter())
end

I love this kind of change. So rhythmic and pleasant to do.

function Monster:getQueryMessage()
    return self.behavior.query(self)
end

Test, commit: Monster uses inherited query.

Loot is a bit more tricky:

function Loot:query()
    self.item:query(self:graphicCenter())
end

That calls this:

function InventoryItem:query(optionalPos)
    if self.name ~= "nothing" then
        Bus:informDotted(self.message, optionalPos)
    end
end

Ah, excellent, we can just rename this here to getQueryMessage and call it. We note, however, that it is going to return a nil sometimes. Either this never occurs, or someone else is handling it. Let’s return an empty string instead, just because our callers have a right to expect a string return.

function InventoryItem:getQueryMessage()
    if self.name == "nothing" then
        return ""
    else
        return self.message
    end
end

And then back up in Loot:

function Loot:getQueryMessage()
    return self.item:getQueryMessage()
end

Test. A test fails:

1: Loot messages  -- 
Actual: , 
Expected: nil

Ah. Find that.

        _:test("Loot messages", function()
            local speaking = Loot(nil, "health", 0,0)
            local silent   = Loot(nil, "nothing", 0,0)
            speaking:query()
            _:expect(busMessage).is("I am a valuable Potent Potion of Health")
            busMessage = nil
            silent:query()
            _:expect(busMessage).is(nil)
        end)

Easily modified. and I found another needing improvement:

        _:test("Loot messages", function()
            local speaking = Loot(nil, "health", 0,0)
            local silent   = Loot(nil, "nothing", 0,0)
            _:expect(speaking:getQueryMessage()).is("I am a valuable Potent Potion of Health")
            _:expect(silent:getQueryMessage()).is("")
        end)
        
        _:test("Can override a Loot's Item's message", function()
            local overridden = Loot(nil, "health", 0,0)
            overridden:setMessage("A new message")
            _:expect(overridden:getQueryMessage()).is("A new message")
        end)

Tests run but I wonder how that setMessage works.

function Loot:setMessage(message)
    self.item:overrideMessage(message)
end

function InventoryItem:overrideMessage(aString)
    self.message = aString
end

We use that in the Learning Level, to give the first thing we find a special message. We’re all good, but I’m still going to test in world.

Works a treat. Commit: Loot uses inherited query.

I noticed something. I’d like for the dialog line to be more prominent. That’s done here:

function Floater:drawMessage(op, pos)
    local txt = op:message()
    local origin = op:speechOrigin(pos)
    strokeWidth(1)
    if origin.x ~= 0 then
        line(origin.x, origin.y, pos.x,pos.y-8)
    end
    text(txt, pos.x, pos.y)
end

I’ll try 2 for strokeWidth. Yes, better. Commit: dialog line strokeWidth is now 2.

Refactoring Done?

Are we done here? I’m not entirely satisfied, because of the NPC. Let’s see what she does:

function NPC:query()
    self.fsm:event("query")
end

That trickles down through the FiniteStateMachine and finally calls publishTale:

function NPC:publishTale(fsm, taleName)
    local tale = fsm.tab.data[taleName]
    local done, msg = tale:next()
    msg = self:name()..": "..msg
    Bus:informDotted(msg, self:graphicCenter())
    return done
end

I’d really like for that to stop doing its own informDotted, instead going up through something in the base class. In the base class we have this:

function DungeonObject:query()
    local msg = self:getQueryMessage()
    local pos = self:getQueryLocation()
    Bus:informDotted(msg, pos)
end

Let’s do this:

function DungeonObject:query()
    local msg = self:getQueryMessage()
    local pos = self:getQueryLocation()
    self:baseQuery(msg,pos)
end

function DungeonObject:baseQuery(msg,pos)
    Bus:informDotted(msg, pos)
end

Now in NPC:

function NPC:publishTale(fsm, taleName)
    local tale = fsm.tab.data[taleName]
    local done, msg = tale:next()
    msg = self:name()..": "..msg
    self:baseQuery(msg, self:graphicCenter())
    return done
end

I’m honestly not sure that I can explain why I prefer that. I think it has something to do with something we’ve not done yet, and I think we’ll find ourselves back here, still dissatisfied.

I should mention that I’m tempted to cause the NPC to adhere more closely to the overall scheme, but I hesitate to mess with the details of the FSM if we don’t need to. It does seem that we could refactor the thing to return the message. However, the real issue is that query is an event to the FSM and it can and does change state. The publication is done via a “callback” in the FSM. We just don’t want to open that can of worms here.

An issue will arise. For now, test … and get a confusing series of errors:

1: Spikes send position parameter -- Entity:23: attempt to call a nil value (method 'baseQuery')

Ah, that’s my message hack, isn’t it, in those new tests that all look alike:

        _:test("Spikes send position parameter", function()
            local fakeSelf = FakeSelf()
            local query = Spikes.query
            query(fakeSelf)
        end)

We patch in the query message to which Spikes will respond … which, by the way, will return the one in the superclass before it gives up looking. But that’s going to look for baseQuery now, in the fakeSelf. So we implement it:

function FakeSelf:baseQuery()
    -- ignored
end

Test run. Let’s commit: Refactor query to call baseQuery, use in NPC. TestQuery tests updated.

I would like to say here that I am not as fond of those TestQuery tests, with the patched query method, as I was yesterday. I think they do exactly what I had in mind, namely check each one to see that it called getQueryLocation and used the result in the informDotted. If any of these objects were overriding query to do something different, I think we’d catch the problem. It just seems more like hackery and less elegant than it did when I thought of it.

But we do have decent tests, they’re just still more intricate than I’d like.

Let’s now see about making the Monster dialog line follow the monster.

Dialog Lines Follow

We’ve just completed a refactoring that has almost everyone (except NPC) calling currentLocation for use in the crawl messages, and all going through the same top level code (except for NPC). One reason why we consolidate code is so that if it has to change, there are fewer places to change. And guess what … we have a change to make.

We want the bus drawing code to call for the position of the dialog line dynamically, so that if the “speaker” moves, the line follows them. Right now it just continues to emanate from where they were when they got the query message.

Again, the drawing code is here:

function Floater:drawMessage(op, pos)
    local txt = op:message()
    local origin = op:speechOrigin(pos)
    strokeWidth(2)
    if origin.x ~= 0 then
        line(origin.x, origin.y, pos.x,pos.y-8)
    end
    text(txt, pos.x, pos.y)
end

To get the origin, we call the OP instance that contains the message. Let’s look in OP first and then see how it gets created.

function OP:speechOrigin(pos)
    return self.speakerPos or vec2(0,0)
end

How did that speakerPos get there? It’s not done in init:

function OP:init(op, receiver, method, arg1, arg2)
    self.op = op
    self.receiver = receiver
    self.method = method
    self.arg1 = arg1
    self.arg2 = arg2
end

There’s something tricky going on here. Look for .speakerPos … we find just this:

function Provider:addTextToCrawl(info)
    local op = OP:display(info.text)
    op.speakerPos = info.speakerPos or vec2(0,0)
    self:privAddItem(op)
end

And addTextToCrawl is a Bus message sent variously, but the case we care about is this:

function EventBus:inform(message,speakerPos)
    if type(message) == "string" then
        message = du.splitStringToTable(message)
    end
    for i,msg in ipairs(message) do
        Bus:publish("addTextToCrawl", {text=msg, speakerPos=speakerPos})
    end
end

function EventBus:informDotted(message, speakerPos)
    Bus:publish("addTextToCrawl", {text="..."})
    self:inform(message,speakerPos)
end

And of course this:

function DungeonObject:baseQuery(msg,pos)
    Bus:informDotted(msg, pos)
end

OK, with that review in mind, what can we do?

How Can We Make speakerPos Dynamic?

One thing we could do would be to pass an object instead of a vector position. The drawCode would send a message to that object rather than just suppose it’s a vector.

Or we could pass in a function and call it. We’d provide a very simple function for the standard case, and a more capable one for Monster. An object would be more, well, object-oriented. Let’s do it that way.

First we’ll start with a refactoring, to keep the same behavior but with a new implementation.

In our function getQueryLocation, we’ll return a tiny object:

function DungeonObject:getQueryLocation()
    return ConstantPosition(self:graphicCenter())
end

Our test will guide us at least part way. This won’t find the class. Lots of tests tell us:

1: Lever puts graphic center into message -- Entity:31: attempt to call a nil value (global 'ConstantPosition')

I’ll write it inline for now:

ConstantPosition = class()

function ConstantPosition:init(aLocation)
    self.loc = aLocation
end

function ConstantPosition:location()
    return self.loc
end

As I sometimes do, I wrote more code than I needed to fix the error, but I am a flawed human being. Test.

1: Lever puts graphic center into message  -- 
Actual: table: 0x292cdc200, 
Expected: (0.000000, 0.000000)

And so on. This is going to be irritating, I think. Let’s see:

        _:test("Lever puts graphic center into message", function()
            local lever = Lever(FakeTile(), "no name")
            _:expect(lever:graphicCenter()).is(vec2(0,0))
            Bus:expect("At first glance, this appears to be a lever.\nDo you think it does anything?", vec2(0,0))
            lever:query()
        end)

We have a fake bus up in here:

function FakeEventBus:inform(message, optionalPos)
    if self.checkPos then
        _:expect(optionalPos).is(self.checkPos)
    end
    if self.checkMessage then
        _:expect(message).is(self.checkMessage)
    end
end

At a guess, we’ve got a ConstantPosition now, so:

function FakeEventBus:inform(message, optionalPos)
    if self.checkPos then
        _:expect(optionalPos:location()).is(self.checkPos)
    end
    if self.checkMessage then
        _:expect(message).is(self.checkMessage)
    end
end

I wonder if we should have called the message position … we’ll hold off on that momentous decision until it works. Test. One more:

1: Key Test  -- 
Actual: table: 0x29221bdc0, 
Expected: (0.000000, 0.000000)

Same thing, I imagine, different object?

function KeyFakeBus:informDotted(msg, gc)
    _msg = msg
    _gc = gc:location()
end

I make that patch. Test. Tests run. I expect the query in the game to crash. It does:

Floater:42: bad argument #1 to 'line' (number expected, got nil)
stack traceback:
	[C]: in function 'line'
	Floater:42: in method 'drawMessage'
	Floater:30: in method 'draw'
	GameRunner:177: in method 'drawMessages'
	GameRunner:140: in method 'draw'
	Main:108: in function 'draw'

We haven’t yet changed the line drawing, which calls down to the OP:

function Floater:drawMessage(op, pos)
    local txt = op:message()
    local origin = op:speechOrigin(pos)
    strokeWidth(2)
    if origin.x ~= 0 then
        line(origin.x, origin.y, pos.x,pos.y-8)
    end
    text(txt, pos.x, pos.y)
end

That leads here:

function OP:speechOrigin(pos)
    return self.speakerPos or vec2(0,0)
end

We have two cases, either speakerPos doesn’t occur or now we need to call it.

function OP:speechOrigin(pos)
    if ~self.speakerPos then return vec2(0,0) end
    return self.speakerPos:location()
end

I expect to be back to normal … except I meant not not ~.

function OP:speechOrigin(pos)
    if not self.speakerPos then return vec2(0,0) end
    return self.speakerPos:location()
end

To my sorrow, it’s not that easy.

CombatRound:225: attempt to call a nil value (method 'location')
stack traceback:
	CombatRound:225: in method 'speechOrigin'
	Floater:39: in method 'drawMessage'
	Floater:30: in method 'draw'
	GameRunner:177: in method 'drawMessages'
	GameRunner:140: in method 'draw'
	Main:108: in function 'draw'

We have other callers, in this case the original announcement. I think the deal is that speakerPos can be nil or a vector or now, an object. Let’s hack a bit.

function OP:speechOrigin(pos)
    if not self.speakerPos then return vec2(0,0) end
    if type(self.speakerPos) == "userdata" then return self.speakerPos end
    return self.speakerPos:location()
end

The “userdata” there is how we detect a vec2. This won’t do, but it does run. So now let’s see about how’s doing the various informs.

The initial message was one that crashed and it works this way:

function GameRunner:initialCrawl(level)
    local co = CombatRound()
    local msgs = self:crawlMessages(level, CodeaUnit)
    local result = {}
    for i,m in ipairs(msgs) do
        table.insert(result, co:display(m))
    end
    return result
end

It’s relying on the combat round to form the op:

function CombatRound:display(aString)
    return OP:display(aString)
end

And that leads to:

function OP:display(aString)
    local op = OP("display")
    op.text = aString
    return op
end

This is irritatingly deep. Leads to provider, I think. But we should find it with our current “display” search. Ah, here it is:

function Provider:addTextToCrawl(info)
    local op = OP:display(info.text)
    op.speakerPos = info.speakerPos or vec2(0,0)
    self:privAddItem(op)
end

We may not need this but we can surely do this:

function Provider:addTextToCrawl(info)
    local op = OP:display(info.text)
    op.speakerPos = info.speakerPos or ConstantPosition(vec2(0,0))
    self:privAddItem(op)
end

Now I should be able to remove that check for userdata. I’ll put an error in during the refactoring, to be sure I find all the cases.

function OP:speechOrigin(pos)
    if not self.speakerPos then return vec2(0,0) end
    if type(self.speakerPos) == "userdata" then error("vector") end
    return self.speakerPos:location()
end

Test. Got this error on purpose, talking to the NPC:

CombatRound:225: vector
stack traceback:
	[C]: in function 'error'
	CombatRound:225: in method 'speechOrigin'
	Floater:39: in method 'drawMessage'
	Floater:30: in method 'draw'
	GameRunner:177: in method 'drawMessages'
	GameRunner:140: in method 'draw'
	Main:108: in function 'draw'

Fix NPC:

function NPC:publishTale(fsm, taleName)
    local tale = fsm.tab.data[taleName]
    local done, msg = tale:next()
    msg = self:name()..": "..msg
    self:baseQuery(msg, ConstantPosition(self:graphicCenter()))
    return done
end

Testing makes me think that we may be done with the refactoring part but let’s look for vec2(0,0), which was a bad thing to do anyway. There seem to be no harmful ones, other than in tests, which do little harm for now. I think we have successfully refactored to use ConstantPosition throughout. But let’s change the method name to position.

function ConstantPosition:init(aPosition)
    self.pos = aLocation
end

function ConstantPosition:position()
    return self.pos
end

function OP:speechOrigin(pos)
    if not self.speakerPos then return vec2(0,0) end
    if type(self.speakerPos) == "userdata" then error("vector") end
    return self.speakerPos:position()
end

function FakeEventBus:inform(message, optionalPos)
    if self.checkPos then
        _:expect(optionalPos:position()).is(self.checkPos)
    end
    if self.checkMessage then
        _:expect(message).is(self.checkMessage)
    end
end

function KeyFakeBus:informDotted(msg, gc)
    _msg = msg
    _gc = gc:position()
end

Test. Hm more failures than I expected. I wish I had a closer save point. Nothing for it but forward now.

No, I’m gonna cherrypick my good changes and then revert the rename.

No. I see the bug. Whew! Failed the rename.

function ConstantPosition:init(aPosition)
    self.pos = aPosition
end

Didn’t you see that? Why didn’t you stop me?

Test. We’re good. Commit: Refactor crawl messages to use an object to get speakerPos ition.

The space was intentional. Tacky but intentional.

Now we can change Monster to return a VariableLocation. And implement it, of course.

function Monster:getQueryLocation()
    return VariablePosition(self)
end

VariablePosition = class()

function VariablePosition:init(anObject)
    self.object = anObject
end

function VariablePosition:position()
    return anObject:graphicsCenter()
end

I expect this to just work. It doesn’t:

Monster:103: attempt to index a nil value (global 'anObject')
stack traceback:
	Monster:103: in function <Monster:102>
	(...tail calls...)
	Floater:39: in method 'drawMessage'
	Floater:30: in method 'draw'
	GameRunner:177: in method 'drawMessages'
	GameRunner:140: in method 'draw'
	Main:108: in function 'draw'

Oh duh. That’s what I get for going fast.

function VariablePosition:position()
    return self.object:graphicsCenter()
end

Still no joy:

Monster:103: attempt to call a nil value (method 'graphicsCenter')
stack traceback:
	Monster:103: in function <Monster:102>
	(...tail calls...)
	Floater:39: in method 'drawMessage'
	Floater:30: in method 'draw'
	GameRunner:177: in method 'drawMessages'
	GameRunner:140: in method 'draw'
	Main:108: in function 'draw'

It’s graphicCenter. Sheesh.

Works. We can commit: Monster query message dialog line follows monster if it moves.

OK, what’s not to like here? Let’s reflect.

Reflection

The two new objects ConstantPosition and VariablePosition are pretty trivial, but the VP certainly could have benefited from a test. And they have no place to live, being written in line just now. So let’s fix that before we forget.

-- Dialog Positions
-- 20220606

function testDialogPositions()
    CodeaUnit.detailed = false
    
    _:describe("Dialog Positions", function()
        
        _:before(function()
        end)
        
        _:after(function()
        end)
        
        _:test("Constant Returns its initial value", function()
            local cp = ConstantPosition(vec2(99,99))
            _:expect(cp:position()).is(vec2(99,99))
        end)
        
    end)
end

That test runs, no big surprise. This one will now but wouldn’t have:

        _:test("Variable changes", function()
            local obj = TestObject(vec2(99,99))
            local vp = VariablePosition(obj)
            _:expect(vp:position()).is(vec2(99,99))
            obj:move()
            _:expect(vp:position()).is(vec2(98,99))
        end)

I need to create my TestObject:

TestObject = class()

function TestObject:init(aVector)
    self.vector = aVector
end

function TestObject:graphicCenter()
    return self.vector
end

function TestObject:move()
    self.vector = self.vector + vec2(-1,0)
end

Test runs. Now let’s move those two little classes into this tab. Tests run. All good. Commit: Move Constant and VariablePosition to own tab with tests.

What else have we to think about here? The NPCs don’t move, but they could. I’ve made a card to note that maybe they should use VariablePosition.

What about those objects? They do the job, but it almost seems as if we could return the basic objects instead, and call graphicCenter instead of position. And we could … but then we’d be passing some pretty broad interfaces around. The little ConstantPosition and VariablePosition objects are what we could think of as a “packet” of information, custom made to a particular purpose.

I could imagine deciding not to use them. In fact, in the version of the program we started with, we didn’t use them, we passed the Whole Darn Thing in a speaker, which led to difficulties with the Loot, which we cold have circumvented differently.

I don’t know. We have two more objects to think about, and they are very specialized, simply used to communicate between the crawl and the objects using it, via the OP. Perhaps we should let them grow up a bit and handle more of the conversation, since the text is passed in separately. We have that odd table info packet that’s being passed around now, and in our preferred style there should probably be an object there.

Remember that the speaker info is actually jammed into the OP, patched in, if you will. We clearly need some kind of polymorphism there, and if we had it, it might subsume the function of these two new objects.

I honestly don’t know if these guys are worthy of life or not. They got me where I wanted to be, and they are quite simple. Maybe as we go forward, we’ll improve them, grow them, get rid of them. It’s all good.

We’re here to make progress in the smoothest way we can. We are not trying to avoid rework, we’re taking small steps, always in the best direction we can see right now. I believe that my understanding and ideas improve over time, so if I have a better idea tomorrow, I’m all for it.

Let’s sum up.

Summary

We refactored the DungeonObject hierarchy to get as much commonality we could, with almost all the code converging up to the top of the hierarchy. Special cases are handled in the subclasses. That is as it should be.

We created two tiny classes with the same simple protocol, one returning a constant answer, the other a variable one, used in moving objects. We created those driven by existing tests and behavior, but could have TDD’s them, were your faithful author not to impatient. I did go back and test them, moving them at the same time into their own tab.

We went through quite a few incremental changes today. There are nine commits, and if I were on my game there would be 11 or 12. We did a commit around every 20 minutes, which is about my sweet spot.

What lessons might we draw? Well, if we must:

Tests are really useful
My existing tests did a decent job of finding most of the problems with the changes. That’s good.

Too much live testing? 2: Crashes found some others immediately. That’s not great because there could have been an obscure case. I’m sure there isn’t, but I could be wrong. I’m not sure how to write the display-level tests, however. That might be a good thing to figure out.

Small improvements add up
The program is a bit better factored and has a nice improvement (fix?) for visible behavior. We find, again, that even with a long-term legacy program, we can make improvements, especially when we have decent tests and fairly well-factored code.
Magic numbers include vectors
My use of vec2(0,0) as a default and in testing would be better if I had a named constant instead of just flinging the value around.
Perfection still a long way off
Perfection still eludes our hero. That leads him to say that readers need not expect perfection from themselves or their colleagues. They might want to pay attention to what happens as they modify how they work, and use that feedback to adjust what they do.
I’m still having fun
I do hope that I think of something new to work on, but meanwhile, this program continues to offer up challenges and opportunities.

I’ll hope to see all three of you next time!