Dungeon 222
That Temporal Coupling bothers me. What are some ways to resolve it?
It’s not a big deal in this design, but it’s a discernible flaw. Maybe by looking at how we can resolve the issue in our tiny lab space, we can prime ourselves with ideas for more important situations.
The problem is this:
When a button or inventory item is touched, we want it to highlight. In addition, if you end the touch still on the thing, we want the thing to do its thing, whatever a button or inventory item does, but if you end the touch having moved off the thing, we want it to take a different action. The button does nothing, the inventory item scrolls a description of itself but does not deploy. In either case, when the touch ends, we want the highlight to end.
So the fact is that we really do want highlighting and action to occur together: the temporal coupling in the UI is desired
- touch begin == highight on
- touch end == highlight off
- inside the object == action one
- outside the object == action two
The issue is in the code. Button deals with the begin this way:
function Button:touched(aTouch, player)
if aTouch.state == BEGAN and self:mine(aTouch) then
Runner:capture(function(aTouch)
self:capturedTouched(aTouch, player)
end)
self.highlight = true
end
end
And InventoryItem:
function InventoryItem:touched(aTouch, pos)
if aTouch.state == BEGAN and manhattan(aTouch.pos,pos) < ItemWidth//2 then
Runner:capture(function(aTouch)
self:capturedTouched(aTouch,pos)
end)
self.highlight = true
end
end
This code is certainly rather significantly duplicated. Now for the end of the touch:
function Button:capturedTouched(aTouch, player)
if aTouch.state == ENDED and player:itsOurTurn() then
if self:mine(aTouch) then
self:performCommand(player)
else
-- lifted outside
end
Runner:capture(nil)
self.highlight = false
end
end
function InventoryItem:capturedTouched(aTouch, pos)
if aTouch.state == ENDED then
if manhattan(aTouch.pos, pos) < ItemWidth//2 then
Inventory:remove(self)
self:informObjectRemoved()
self:usedMessage()
Bus:publish(self.attribute, self.value1, self.value2)
else
self:informObjectTouched()
end
Runner:capture(nil)
self.highlight = false
end
end
Again, the code is very similar in shape. However, the biggest code smell, to me, is the need to set and clear highlight, particularly the clear, which would be easy to forget. It’s also possible to fail to clear the capture, and in fact at one point yesterday there was a path that did retain the capture after it should have released it.
Currently, as you can see in the code above, I’m1 doing the capture by saving a function in GameRunner, that is called in the touch logic. Each of the things that can capture the touch provides a function that is called back. They both happen to call capturedTouch but could in fact do anything.
Yesterday at article end, I had no good idea how to improve this. During the day, I had an idea for how to make it better. I vaguely remember what it was.
The idea was to have an object, just one (singleton? no, just a global) that receives all touch commands, and that encapsulates all the state needed to manage both the highlighting and command execution. That object could essentially “translate” touch events into ones that capture, ones that are ignored, and ones that cause one or another action.
At first this seemed like a pretty good idea–and it still may be–but there are issues. One issue is that the InventoryItems don’t know their position, so they can’t determine on their own whether they are touched or not. We’d have to resolve that somehow, perhaps by just updating their positions whenever they move.
A related issue is that we’d have to provide this touch handler object some way of talking to all the objects that might receive the touch. There are buttons, which are known to the GameRunner, and InventoryItems, which are known to Inventory, which is known to GameRunner. So assembling these objects would have to be done somehow.
Another issue, smaller, is that all the objects that can be touched would have to have a method to highlight or not highlight. No big thing, and arguably we should have been using those already.
Later yesterday, or early this morning, it occurred to me that the EventBus could help. If all the touchable objects were to subscribe to a suitable event, when a touch occurred, the one touched could call the touch handler and say “it’s me”. The handler could save the object, and when the touch ends, ask it whether that touch is inside, and … no, better: when the touch ends, send the object a standard message including the current touch position, and the object decides what to do.
(We might argue that it would be better to have the handler determine whether the touch was inside, but that seems more invasive to me. The code will tell us.)
I think it’s worth pointing something out. As you can see two paragraphs above, I’m still designing, right in the middle of the story. I’m not committing to yesterday’s idea, or this morning’s idea. I’m holding on to them loosely, letting the solution take shape as it will, as we learn about it by doing it.
In short: never stop thinking.
A Tentative Plan
I’m ready to sketch some steps toward doing this improvement.
- Buttons and InventoryItems will need to have a method to set highlighting on and off.
- InventoryItems will need to know their own position.
Speaking of never stop thinking, why is there a difference between an InventoryItem and a Button? Why isn’t an InventoryItem a kind of Button, or held onto by a kind of Button that has a particular graphical appearance? If we did that, we might be able to converge some behavior between the two.
I’m going to set this idea aside, but I am making a card for it to consider later.
First let’s provide and use setHighlight in both classes.
You see, whenever we say something like that, doing something in more than one place, it’s a dead giveaway that there is a commonality of behavior that is not currently being exploited. Two things doing the same thing isn’t as good as one thing doing the thing for both of them.
function Button:capturedTouched(aTouch, player)
if aTouch.state == ENDED and player:itsOurTurn() then
if self:mine(aTouch) then
self:performCommand(player)
else
-- lifted outside
end
Runner:capture(nil)
self:setHighlight(false)
end
end
function Button:setHighlight(aBoolean)
self.highlight = aBoolean
end
function Button:touched(aTouch, player)
if aTouch.state == BEGAN and self:mine(aTouch) then
Runner:capture(function(aTouch)
self:capturedTouched(aTouch, player)
end)
self:setHighlight(true)
end
end
Is that all of them? Better check init. It’s not initialized but probably should be. Unitialized is false, but is that tacky? I think it is.
function Button:init(name, x,y,w,h, img)
self.name = name
self.x = x
self.y = y
self.w = w
self.h = h
self.img = img
self:setHighlight(false)
end
Now the InventoryItem similarly.
Simple refactoring, all works as before. Commit: created setHighlight for Button and InventoryItem.
Why commit this? Because it’s complete, and so if the thunderstorm cuts me off from the repo, or I decide to take the rest of the day off, my changes are all in, and the system can ship if it wants to. And because, when I do small commits I can just revert a mistake, and the longer I wait, the longer I’m willing to debug rather than just start a few lines over.
Now let’s have the InventoryItems know their own position. We’ll give them a member variable and setter:
function InventoryItem:init(aString, aValue2)
local entry = InventoryTable:getTable(aString)
self.icon = entry.icon
self.name = entry.name or self.icon
self.description = entry.description or self.name
self.used = entry.used or nil
self.attribute = entry.attribute or nil
self.value1 = entry.value or entry.value1 or nil
self.value2 = aValue2 or entry.value2 or nil
self.position = nil
end
function InventoryItem:setPosition(position)
self.position = position
end
Now to set it, which needs to be done in Inventory. This might be a bit tricky. I didn’t look, so this is as much of a surprise to me as it probably is to you. We’ll want to set position when things are added or removed:
function Inventory:add(item)
self:put(self:get(item):increment())
end
function Inventory:remove(item)
self:put(self:get(item):decrement())
end
function Inventory:put(entry)
if entry.count > 0 then
inventory[entry.name] = entry
else
inventory[entry.name] = nil
end
self:makeLinear()
end
function Inventory:makeLinear()
linear = {}
for name,entry in pairs(inventory) do
table.insert(linear, entry)
end
end
That last bit is odd. Has no possible impact. Remove it, or leave it for later? Commit: add setPosition method to inventory item.
Remove it and see if we die. Can’t possibly die.
We die. The linear table is local to Inventory and is used to provide a list of entries. We’ll not mess with that.
See why we commit often? Revert.
Should have minded my own business. We should update the positions at the end of put.
function Inventory:put(entry)
if entry.count > 0 then
inventory[entry.name] = entry
else
inventory[entry.name] = nil
end
self:makeLinear()
self:setPositions()
end
I remember now. The
linearis used to keep inventory items displaying in a more or less consistent order. If we kept them in a hash table, they’d move around in the display. Anyway …
Forgive me, I just looked at some other code and typed this in:
function Inventory:setPositions()
local count = self:count()
for i,e in pairs(self:entries()) do
local pos = self:drawingPos(i, count)
e:setPosition(pos)
end
end
The handy function drawingPos was there for the using.
Now they should all know their position, but not use it.
Ewww. If we wind up using the EventBus, the fully removed ones will need to clear their access to the bus. I think we’ll go another way. Maybe only Inventory concerns itself with the bus. And if that’s the case, we won’t need to keep position in the individual items at all.
Yes, the code is telling me to change the design. I could force it to work according to my plan, or I can let the code participate in the thinking. Let’s remove the position stuff entirely and start from there.
Revert the new setPositions, undo previous commit, undo that. We’re back with setHighlight and that’s it.
Learning: there’s a way that seems better than requiring InventoryItems to know their own positions, which are dynamic, and to avoid their need to subscribe to touch events.
Let’s plan the next steps:
A Further Tentative Plan
Let’s see. In Main is where the touching starts:
function touched(aTouch)
CodeaTestsVisible = false
Runner:touched(aTouch)
end
Runner loops over buttons and tells inventory to loop over buttons. Let’s just assume that’s not happening, and let’s code up what we need as part of Main. We’ll surely want a TouchHandler class to do this, won’t we? We’ll see.
What we want to do breaks down into touch BEGAN and touch ENDED.
On BEGAN, we want to publish, I don’t know, “touchBegan”, with the position and whatever else we need. We will provide some way for whoever recognizes the touch position will identify itself to us using the parameters on the event. We’ll remember the object and send it setHighlight(true).
On ENDED … we’ll do something. I don’t want to decide yet.
What are the parameters to publishing?
function EventBus:publish(event, optionalSender, optionalInfo1, optionalInfo2)
for subscriber,method in pairs(self:subscriptions(event)) do
method(subscriber, event, optionalSender, optionalInfo1 or {}, optionalInfo2 or {})
end
end
We can send in any object and any two parameters. The subscriber specifies the method to be called on the event, and decodes the object and parameters any way it wants.
We have to pass the touch position, and I propose to pass in a function for the subscriber to call. For now, a plain vanilla function:
function capture(anObject)
TouchCaptured = anObject
end
Somewhere along here we’re going to have to break things, but not quite yet.
function touched(aTouch)
CodeaTestsVisible = false
Bus:publish("touchBegan", nil, aTouch.pos, capture)
Runner:touched(aTouch)
end
This is harmless, no one is listening. Now, in Button:
function Button:init(name, x,y,w,h, img)
self.name = name
self.x = x
self.y = y
self.w = w
self.h = h
self.img = img
self:setHighlight(false)
Bus:subscribe(self, self.touchBegan, "touchBegan")
end
function Button:touchBegan(event, sender, pos, callback)
if self:isPosMine(pos) then
callback(self)
end
end
function Button:isPosMine(pos)
local x = pos.x
local y = pos.y
local ww = self.w/2
local hh = self.h/2
return x > self.x - ww and x < self.x + ww and
y > self.y - hh and y < self.y + hh
end
Now I expect that the buttons will call back. I have no good test for this, and I really should have. But I’m in the middle of stringing a wire across the chasm. I might have done well to start with a test. Now, I’m going to hang out over the chasm until I get across or fall in. If I fall in, I’ll start with tests. If not, well, no promises. I’m human.
I’m going to slap a print into that function over in main.
function capture(anObject)
print("captured ", anObject)
TouchCaptured = anObject
end
If this works at all, it’ll spam the print but that’s OK as a test.
Two tests fail looking for Bus, I’ll deal with them shortly.
The print does get spammed, so I’m rather certain that my capture is coming through. Now let’s break things.
function capture(anObject)
TouchCaptured = anObject
TouchCaptured:setHighlight(true)
end
function touched(aTouch)
CodeaTestsVisible = false
if aTouch.state == BEGAN then
Bus:publish("touchBegan", nil, aTouch.pos, capture)
elseif aTouchState == ENDED then
TouchCaptured:setHighlight(false)
TouchCapture = nil
end
--Runner:touched(aTouch)
end
Now I expect the buttons to highlight and do nothing, and the inventory items won’t even highlight,
Curiously, the buttons do highlight, but they don’t unhighlight.
Duh! aTouchState is meaningless. Darn auto-globals.
function touched(aTouch)
CodeaTestsVisible = false
if aTouch.state == BEGAN then
Bus:publish("touchBegan", nil, aTouch.pos, capture)
elseif aTouch.state == ENDED then
TouchCaptured:setHighlight(false)
TouchCaptured = nil
end
--Runner:touched(aTouch)
end
Now they’d better highlight on and off! Well, needed to check to be sure something was captured on ENDED, lest we send messages to nil:
function touched(aTouch)
CodeaTestsVisible = false
if aTouch.state == BEGAN then
Bus:publish("touchBegan", nil, aTouch.pos, capture)
elseif TouchCaptured and aTouch.state == ENDED then
TouchCaptured:setHighlight(false)
TouchCaptured = nil
end
--Runner:touched(aTouch)
end
Now we want to send a standard message to the captured object telling it that the touch is ended and it’s your job to deal.
Let’s see what button has. I’m thinking that capturedTouched method is just the thing to study:
function Button:capturedTouched(aTouch, player)
if aTouch.state == ENDED and player:itsOurTurn() then
if self:mine(aTouch) then
self:performCommand(player)
else
-- lifted outside
end
Runner:capture(nil)
self:setHighlight(false)
end
end
We won’t be using that at all. We should pass in the position, not the touch, as the new method needs no knowledge of the rest of the touch. But what about player? Runner knows it. I’m going to just demand it for now:
function touched(aTouch)
CodeaTestsVisible = false
if aTouch.state == BEGAN then
Bus:publish("touchBegan", nil, aTouch.pos, capture)
elseif TouchCaptured and aTouch.state == ENDED then
TouchCaptured:touchEnded(aTouch.pos, Runner.player)
TouchCaptured:setHighlight(false)
TouchCaptured = nil
end
--Runner:touched(aTouch)
end
And write that function:
function Button:touchEnded(pos, player)
if self:isPosMine(pos) then
self:performCommand(player)
end
end
I expect the buttons to work again. They do.
We can’t commit yet, because we’ve broken Inventory.
We can, however, strip some excess out of buttons.
And we have some broken tests to fix:
This is taking too long. Last commit was 0855, it’s 1010 now. I’ll give myself to 1030 and then demand a revert and start over. No pressure, just seems long enough.
1: Lift Test -- Actual: function: 0x2827a2c00, Expected: function: 0x2827d3600
1: Lift Test -- Button:144: attempt to call a nil value (method 'touched')
2: Touch Test -- Actual: function: 0x2827a2c00, Expected: function: 0x2827d3600
2: Touch Test -- Button:153: attempt to call a nil value (method 'touched')
I think we have seriously obsoleted these tests. Here they are:
_:test("Lift Test", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(200,200), state=ENDED}
_:expect(b:mine(t)).is(true)
b:touched(t, _player)
_handler(tEnd, _player)
end)
_:test("Touch Test", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(105,105), state=ENDED}
_:expect(b:mine(t)).is(true)
b:touched(t, _player)
_:expect(_fooCalled).is(false)
_handler(tEnd, _player)
_:expect(_fooCalled).is(true)
end)
Hey! These would have been useful in what we just did. Dullard! :)
I already, at 1014, think I need a time extension here. We’ll see.
These tests aren’t even close to what we need. I’ll remove some failing bits and see if we can get to a commit point.
_:test("Lift Test", function()
--[[
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(200,200), state=ENDED}
_:expect(b:isPosMine(t.pos)).is(true)
b:touched(t, _player)
_handler(tEnd, _player)--]]
end)
_:test("Touch Test", function()
--[[
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(105,105), state=ENDED}
_:expect(b:isPosMine(t.pos)).is(true)
b:touched(t, _player)
_:expect(_fooCalled).is(false)
_handler(tEnd, _player)
_:expect(_fooCalled).is(true)--]]
end)
I can’t ignore them and then commit, so I saved the bits that need replication with a new plan.
T = 1022
Buttons are working, inventory isn’t. So … what if we did this:
It’s 1025 and I’m sure I can’t make 1030. Should the big fool press on, or revert? What we have is decent. Not finished product, but decent. Let’s push on. We’ll sum up honestly anyway, we always do.
Yikes, look at this:
function Inventory:init()
assert(false, "do not instantiate inventory")
end
I forgot that Inventory runs everything on its class side. Where can I stand to give it a bus subscription?
For now:
function Inventory:subscribe()
Bus:subscribe(Inventory, Inventory.touchBegan, "touchBegan")
end
function Inventory:touchBegan(event, sender, pos, callback)
for i,entry in ipairs(self:entries()) do
if entry:isPosMine(pos) then
capture(entry)
return
end
end
end
And to call the subscribe … I’ve put it in Main:setup.
Test, this may crash in a useful way.
It doesn’t …
I am so inclined to capitulate and revert. But the problem is, the button stuff is good. It deserves to be committed but the Inventory part doesn’t work.
I’m going to leave my branch open and take a short break, then decide. If I were actually on a branch, I’d definitely commit it.
Let’s Regroup
I’ve got some half-bun changes in Inventory Tab, but the changes in Main and Button are pretty righteous. Let’s see if I can convince Working Copy to give me back the old Inventory.
Yes, by golly, you can go into the file list, find Inventory, revert it with some warnings.
Let’s see what we have now.
Main:44: attempt to call a nil value (method 'subscribe')
stack traceback:
Main:44: in function 'setup'
Right. Give Inventory a subscribe method. I’m regretting not making it an instance after all.
function Inventory:subscribe()
Bus:subscribe(Inventory, Inventory.touchBegan, "touchBegan")
end
function Inventory:touchBegan(event, sender,pos,callback)
print(event, sender, pos, callback)
end
I have a feeling that the parameters here aren’t what I think they are, so I’ve decided to print them.
touchBegan nil (726.500000, 987.500000) function: 0x28332c060
After some confusion, I decide that’s OK, but we aren’t getting the sender. I guess that’s OK because the callback is a plain function.
So let’s find who likes this position, if anyone:
function Inventory:touchBegan(event, sender,pos,callback)
local count = self:count()
for i, entry in pairs(self:entries()) do
local entryPos = self:drawingPos(i,count)
if entry:isPosMine(pos, entryPos) then
callback(entry)
end
end
end
This should fail looking for isPosMine.
Inventory:210: attempt to call a nil value (method 'isPosMine')
stack traceback:
Inventory:210: in local 'method'
EventBus:112: in method 'publish'
Main:104: in function 'touched'
So far so good.
function InventoryItem:isPosMine(aPos, myPos)
local d = manhattan(aPos,myPos)
return d < ItemSize//2
end
Now that should capture the Item. What will Main do?
function touched(aTouch)
CodeaTestsVisible = false
if aTouch.state == BEGAN then
Bus:publish("touchBegan", nil, aTouch.pos, capture)
elseif TouchCaptured and aTouch.state == ENDED then
TouchCaptured:touchEnded(aTouch.pos, Runner.player)
TouchCaptured:setHighlight(false)
TouchCaptured = nil
end
--Runner:touched(aTouch)
end
I see no touchEnded in InventoryItem so that should be the error.
Inventory:210: attempt to call a nil value (method 'isPosMine')
stack traceback:
Inventory:210: in local 'method'
EventBus:112: in method 'publish'
Main:104: in function 'touched'
That’s the same message. What is being sent that message?
Ah. The inventory doesn’t contain InventoryEntry objects, it contains InventoryEntry objects, which contain an InventoryItem and a count.
This is what was stopping me before, Should be an easy fix.
However, I’m still not sure we can get a line all the way across the chasm. We’re capturing the InventoryEntry now, so we can put the isPosMine method there and forward:
function InventoryEntry:isPosMine(...)
return self.item:isPosMine(...)
end
Now let’s see a more suitable failure:
Inventory:324: attempt to perform arithmetic on a nil value (global 'ItemSize')
stack traceback:
Inventory:324: in function <Inventory:322>
(...tail calls...)
Inventory:215: in local 'method'
EventBus:112: in method 'publish'
Main:104: in function 'touched'
Variable is ItemWidth, not Size.
Main:98: attempt to call a nil value (method 'setHighlight')
stack traceback:
Main:98: in function 'capture'
Inventory:216: in local 'method'
EventBus:112: in method 'publish'
Main:104: in function 'touched'
Let’s play this through and see what happens. Forward:
function InventoryEntry:setHighlight(...)
return self.item:setHighlight(...)
end
I kind of like this notion of forward, where we don’t think about the arguments or return, we just forward and return everything.
Now what breaks?
Main:106: attempt to call a nil value (method 'touchEnded')
stack traceback:
Main:106: in function 'touched'
OK, what should an InventoryEntry to about touchEnded?
We need to have the equivalent of this happen:
function InventoryItem:capturedTouched(aTouch, pos)
if aTouch.state == ENDED then
if manhattan(aTouch.pos, pos) < ItemWidth//2 then
Inventory:remove(self)
self:informObjectRemoved()
self:usedMessage()
Bus:publish(self.attribute, self.value1, self.value2)
else
self:informObjectTouched()
end
Runner:capture(nil)
self:setHighlight(false)
end
end
Except without all the frills. So let’s do touchEnded with the right stuff and then forward. What does the method expect?
TouchCaptured:touchEnded(aTouch.pos, Runner.player)
OK …
function InventoryItem:touchEnded(aPos, player, myPos)
if self:isMyPos(aPos,myPos) then
Inventory:remove(self)
self:informObjectRemoved()
self:usedMessage()
Bus:publish(self.attribute, self.value1, self.value2)
else
self:informObjectTouched()
end
end
But we need to be passed our position. So let’s have InventoryEntry get that:
function InventoryEntry:touchEnded(aPos, player)
local myPos = Inventory:myPos(self)
return self.item:touchEnded(aPos,player,myPos)
end
Now Inventory needs to help me out here:
function Inventory:myPos(entry)
local count = self:count()
for i, e in ipairs(self:entries()) do
if e==entry then return self:drawingPos(i,count) end
end
return vec2(10000,10000)
end
This is messy. I’m still holding on by my fingernails, but at least I know it. Might work. Let’s find out.
Nope:
Inventory:316: attempt to call a nil value (method 'isMyPos')
stack traceback:
Inventory:316: in function <Inventory:315>
(...tail calls...)
Main:106: in function 'touched'
Method is isPosMine. Bad name? Probably.
It works!
I think we are even correctly wired up. I( should do a bit more in-game testing, unfortunately.
Game seems solid. Commit: New Bus-based touch handling works for both Button and Inventory.
Let’s retrospect:
Retro
It’s 1150, so we are essentially three hours in. Intuitively I think we should have completed in two hours or less, for values of “should”. But facts are facts, so what I’m saying is that had I proceeded as I do at my best, I’d have saved an hour.
What did I do that could have actually been better? Well, I could have insisted on test-driving the changes. That would at least have saved me a bunch of time running the game, even if it didn’t get me to the answer sooner.
What I did that couldn’t have been better was that I forgot that the Inventory contains InventoryEntry objects, not InventoryItem objects. That cost me some time, because I didn’t know why it wasn’t working, and caused me, finally, to revert out and start over.
Starting over, I went in smaller steps and somehow did rediscover the InventoryEntry.
What I might have done that would have helped would have been to review the Inventory code more carefully, so that I’d have noticed the InventoryEntry. But that’s not good advice, because I am as careful as I am, and I’m not likely to get more careful.
But I can change what I do, if not how I am, and I can certainly lift up my inclination to to TDD.
Again. Yes, I know it’s again. Good habits often need repeated rededication. Unless I could change Codea to refuse to run without a test for whatever I was doing, I’ve got to adjust my own behavior.
The second time went very well. If I could just write up the Buttons, which went nicely, and the second pass at Inventory, I’d look pretty good.
That’s how most programming books are written, by the way. The author tries things until they find something they consider right, then they write it up as if it were obvious, sort of thing we all do all the time, right off the cuff don’t you know. Yeah, right. Trust me, no one is that good.
I’m just dumb enough to document my mistakes. I do it so that you won’t feel lonely if you ever make a mistake.
OK, so it went a bit roughly, but not too badly.
I still feel fairly fresh. Let’s tidy up tag ends of code and maybe, just maybe, we can make those two commented tests work.
To focus my attention on that, I’ll change them to ignore, so that I’ll have to do something before I can do another commit. OK, that’s done, but first I want to root out code that’s no longer used.
function GameRunner:basicTouched(aTouch)
for i,b in ipairs(self.buttons) do
b:touched(aTouch, self.player)
end
Inventory:touched(aTouch)
end
function GameRunner:touched(aTouch)
self.touchHandler(aTouch)
end
function Inventory:touched(aTouch)
for i,entry in ipairs(self:entries()) do
entry.item:touched(aTouch, self:drawingPos(i, self:count()))
end
end
function InventoryItem:touched(aTouch, pos)
if aTouch.state == BEGAN and manhattan(aTouch.pos,pos) < ItemWidth//2 then
Runner:capture(function(aTouch)
self:capturedTouched(aTouch,pos)
end)
self:setHighlight(true)
end
end
function InventoryItem:capturedTouched(aTouch, pos)
if aTouch.state == ENDED then
if manhattan(aTouch.pos, pos) < ItemWidth//2 then
Inventory:remove(self)
self:informObjectRemoved()
self:usedMessage()
Bus:publish(self.attribute, self.value1, self.value2)
else
self:informObjectTouched()
end
Runner:capture(nil)
self:setHighlight(false)
end
end
I’ll test this and see if I’ve been too aggressive. Seems good. There must be more in Runner, at least the capture stuff. Yes, another tiny method and a call to it.
This is enough that I want to commit it. Let’s see if we can deal with those tests or what.
_:ignore("Lift Test", function()
--[[
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(200,200), state=ENDED}
_:expect(b:isPosMine(t.pos)).is(true)
b:touched(t, _player)
_handler(tEnd, _player)--]]
end)
_:ignore("Touch Test", function()
--[[
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(105,105), state=ENDED}
_:expect(b:isPosMine(t.pos)).is(true)
b:touched(t, _player)
_:expect(_fooCalled).is(false)
_handler(tEnd, _player)
_:expect(_fooCalled).is(true)--]]
end)
Now one issue here is that these tests need an event bus. We have a Fake one but it’s not rigged for these tests. Let’s see if we can just finesse it.
_:before(function()
_bus = Bus
Bus = {subscribe=function() end}
_captureCount = 0
_hander = nil
_fooCalled = false
_runner = Runner
_player = {itsOurTurn=function(_self)
return true
end,
foo=function() _fooCalled = true end,
turnComplete=function() end
}
Runner={capture=function(_self, aFunction)
if _captureCount%2 ==0 then
_handler = aFunction
_:expect(type(_handler), "first time").is("function")
else
handler = nil
_:expect(aFunction, "second time").is(nil)
end
_captureCount = _captureCount + 1
end
}
end)
One method, subscribe, does nothing. Sweet. Tests now run this far:
_:test("Lift Test", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(200,200), state=ENDED}
_:expect(b:isPosMine(t.pos)).is(true)
--[[
b:touched(t, _player)
_handler(tEnd, _player)--]]
end)
_:test("Touch Test", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(105,105), state=ENDED}
_:expect(b:isPosMine(t.pos)).is(true)
--[[
b:touched(t, _player)
_:expect(_fooCalled).is(false)
_handler(tEnd, _player)
_:expect(_fooCalled).is(true)--]]
end)
I think we might replace the touch / _handler check with a call to the event handler, which as a test we’re allowed to know about:
function Button:touchBegan(event, sender, pos, callback)
if self:isPosMine(pos) then
callback(self)
end
end
Well, this is nice, we can pass in our own callback.
I rename the test:
_:test("Callback Test", function()
local callbackButton = nil
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
local tEnd = {pos=vec2(200,200), state=ENDED}
_:expect(b:isPosMine(t.pos)).is(true)
b:touchBegan(event, sender, t.pos, function(button)
callbackButton = button
end)
_:expect(callbackButton).is(b)
end)
Now we really want two more tests, one where the new position is inside, and one where it isn’t.
In the Ended test, I do this much:
_:test("Touch End", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
_:expect(b:isPosMine(t.pos)).is(true)
b:touchEnded(vec2(104,106), t.pos)
The error is:
2: Touch End -- Button:79: attempt to call a nil value (field '?')
That’s here:
function Button:performCommand(player)
player[self.name](player)
player:turnComplete()
end
We’re gonna need to pass in the player. We do have a fake one set up:
_player = {itsOurTurn=function(_self)
return true
end,
foo=function() _fooCalled = true end,
turnComplete=function() end
}
We’ll pass it and see …
_:test("Touch End", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
_:expect(b:isPosMine(t.pos)).is(true)
b:touchEnded(vec2(104,106), _player)
Test runs. The player is checking to be sure that foo is called. Let’s be sure we’ve initialized that flag … no wait, the rest of the test tells a story:
_:test("Touch End", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
_:expect(b:isPosMine(t.pos)).is(true)
b:touchEnded(vec2(104,106), _player)
--[[
b:touched(t, _player)
_:expect(_fooCalled).is(false)
_handler(tEnd, _player)
_:expect(_fooCalled).is(true)--]]
end)
We want to end first outside, then inside. Let’s replicate that:
_:test("Touch End", function()
local b = Button("foo", 100,100, 20,20)
local t = {pos=vec2(105,105), state=BEGAN}
_fooCalled = false
_:expect(b:isPosMine(t.pos)).is(true)
b:touchEnded(vec2(200,200), _player)
_:expect(_fooCalled).is(false)
b:touchEnded(vec2(104,106), _player)
_:expect(_fooCalled).is(true)
end)
Tests both run. Commit: removed dead code, made button tests run.
Let’s sum up!
Summary
Well, a bit of thrashing. Arguably working from the Button tests would have helped, but Button went smoothly anyway. And writing Inventory tests following the lines of the Button tests we now have would probably have made that go a lot better as well.
But it wasn’t awful and the rabbit hole we fell into wasn’t all that deep.
Looking at it now, we see two simple methods, touchBegan and touchEnd that objects implement to be treated as buttons. The calling sequence is a bit odd, because of the need to know the player, but that’s not the fault of the buttons, it’s a more fundamental flaw in our design, where we’ve tried to limit globals and wound up with GameRunner knowing way too much.
Possibly we could have refactored our way to here, never breaking anything, had we created those two methods “first”, before starting the other touch work, but I think there were just too many things to rewire in Inventory for it to have been truly easy.
We still have duplication, those two methods and their friends that check position and such. It’s certainly possible, maybe even probable, that if we had started by making inventory entries be buttons, we’d have been better off.
There’s no way to know, but today was ragged enough to make us want to remember to look around a bit more, make smaller plans, write more tests.
But here’s the tricky one: we started with the easy case, Buttons. Correction: I started with Buttons. Generally I like to start with the easy case because I learn more readily what needs to be done. But Inventory was enough different that the learning wasn’t enough.
I’m not thinking that I’ll cut over to starting with hard stuff. No good can come of that. But I do think we could have drawn Inventory and Button closer together in style, to our advantage. Refactoring Inventory in the direction of Button, if not to actually be buttons, might have helped.
Hindsight, and it’s still not 20-20.
I feel that I wrestled this one to the ground, rather than gracefully throwing it over my shoulder, but all’s well that ends.
See you next time!
-
As usual, I say “I” when I’m talking about something maybe not so good, accepting any blame necessary, and I say “we” when I hope we’re working together. I’m sure you’d have helped me do better yesterday if you were really here. ↩