Dungeon 10
Is there a difference between creating a better design and discovering one? I think so.
Yesterday’s article was long, and probably boring to read, even for the few people that read them. The experience of doing the work was quite pleasant, and rather different from much of what I experience as I program.
Now I try to be sensitive to whether I’m feeling tense, and when I am, I try to slow down and take smaller bites. If there’s a revert point not too far back, I try to get myself to use it. Of course, I do get that teeth-gritting feeling that dammit if I just dig a little further I’ll crack the problem–and sometimes that even works.
Yesterday, I felt almost no tension during that whole session. It went very smoothly, and that’s a feeling I’d like to be able to produce at will.
Yesterday started with extending the door logic to work in all orientations, and I intentionally let duplication arise, because I’m always confident that duplication can be removed with a resulting improvement in the design. The process of removing duplication is generally very simple and straightforward, consisting mostly of extracting common elements and using them wherever the duplication formerly existed.
Without a refactoring tool, refactoring is always a bit more risky than one would like, especially in a language like Codea Lua, where a typo in a variable name goes without comment until that variable is used. But I like to refactor by hand anyway, as it kind of focuses my mind in a mellow sort of way. Most of the changes are easy and one just steps through them 1,2,3.
But tests are critical to refactoring, and nowhere more critical than in a language like Lua and a system like Codea with no type checking and no refactoring support. I’ve had difficulty getting decent tests in place in Codea, because so much of what one does is on the screen, and it’s easier to look and see than it is to compute that the object should now be at x = 123.68.
And even for someone who has been doing TDD for over 20 years, I do find that my mind often jumps to the code, not to the test for the code, and it’s easy to fall into the trap of doing things without tests. And you do usually get away with it. After all, we’re all good programmers here, and we can create code that works, without tests, if we’re careful.
Fortunately–and I use the word advisedly–I have decent tests. It just kind of turned out that way. I try to lean toward writing tests, but in the case of Dungeon, I also wrote some because the left right up down east west north south stuff is just confusing enough that I needed specific examples.
I think I just realized why I confuse left and right east and west so often. When I mention left and right, I will almost always mention left, then right. When I mention east and west, I almost always mention east first, then west. Those two lists don’t match. I should say left right west east or right left east west, shouldn’t I? If I trained myself to do that, would it help? I wonder.
The bulk of yesterday’s work, and there was a lot of it, was in refactoring toward a better design. I had a simple idea, to create an object that knew about the end points of the overlapping bits of two rooms, and used that to determine whether rooms had enough overlap to have doors between them, and if so, where the door should be.
My original code for that had a lot of right left east west up down north south going on. The tiny insight was that since the overlap only considers the end points of segments, it doesn’t really care whether it’s a y-oriented segment or an x-oriented one. So it was a small but decent idea.
The work consisted of converting the separate x-y methods in Room to use this new object, by incrementally creating generic methods in the SegmentPair, and just calling back to the original method, then pulling the implementation over. Pulling the implementation over removed the concern over x and y, because SegmentPair doesn’t care about orientation except when it’s created. So little by little, two methods in Room turned into one method in SegmentPair.
It went very smoothly, consisting of the same steps over and over, moving code kind of bottom up into SegmentPair, changing the names from xSomeThing ySomeThing into someThing`, because SegmentPair, like Honey Badger, doesn’t care. Test, rinse, repeat.
What was different about this from most of my duplication-removal refactoring was that the changes were all directed toward a single “large refactoring”, the replacement of all the x and y calls about overlap into a single amazing object, SegmentPair, that was dimension free. And while it wasn’t replacing 10,000 lines of code with 5,000, it did probably replace two or three hundred with about one hundred, and it did it all in tiny steps with all the tests running.
The trick was the call-back, the double dispatch as it is called. The new object was created with access to all the information that the old code had, and when the new object didn’t yet have the ability to do things in the new way, it just called the old methods. Then, one by one, the old got replaced with the new.
I really didn’t know what the SegmentPair was going to look like when it was done, and when I first looked at the code this morning I thought some updates must have been missing, because it has become so simple and compact.
Discovering versus Creating
In essence, I discovered this new design. I didn’t sit down and draw diagrams and plan member variables and methods and connections. I just slowly moved capabilities from one place to another, much as I do in all refactoring. The difference was that I was focused on this one new place, SegmentPair, rather than the more casual refactoring that either creates a new method in the class we’re in, or sometimes pushes a method over to some other class due to feature envy.
This was more continuous, but amazingly smooth and simple. I could have stopped and shipped at any point, because the tests were always working, even when the new object was only fractionally complete.
It was very cool, and I hope you can get enough of a sense of it to give it a go.
Now for today.
Today
Today is Veteran’s Day (thank you all for your service), and my wife is off work, so we’ll go to the grocery store real soon now, for cat food and other important matters, including my chai. That aside, I have plans for the code today.
First on the agenda is this:
function Room:yOverlapCenter(aRoom)
local sp = SegmentPair:forY(self,aRoom)
local lo,hi = sp:overlapLoHi()
return (lo + hi)//2
end
function Room:xOverlapCenter(aRoom)
local sp = SegmentPair:forX(self,aRoom)
local lo,hi = sp:overlapLoHi()
return (lo + hi)//2
end
function Room:xOverlapLength(aRoom)
local sp = SegmentPair:forX(self,aRoom)
local lo,hi = sp:overlapLoHi()
return math.max(hi-lo,0)
end
function Room:yOverlapLength(aRoom)
local sp = SegmentPair:forY(self,aRoom)
local lo,hi = sp:overlapLoHi()
return math.max(hi-lo,0)
end
These are the last x-y methods in Room, and I think for now they need to continue to exist, because they’re used here:
function Room:isNeighbor(aRoom)
if self == aRoom then return false end
local doorWidth = 10
return (self:nsCloseEnough(aRoom) and self:xOverlapLength(aRoom)>doorWidth) or
(self:ewCloseEnough(aRoom) and self:yOverlapLength(aRoom)>doorWidth)
end
And here:
function Neighbor:getDoor(aRoom)
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
local y = aRoom:yOverlapCenter(otherRoom)
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
local x = aRoom:west()
local y = aRoom:yOverlapCenter(otherRoom)
return x,y
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:south()
return x,y
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:north()
return x,y
else
return nil
end
end
Now those look like candidates for improvement, especially that last thing. You’ll recall that I wrote that out fully longhand on purpose, to expose the duplication. Duplication, of course, consists of elements that are held in common in the various duplicated bits. They are ideas that have not yet been fully expressed. Much of refactoring, especially “Extract Method”, amounts to recognizing these ideas, giving them names, and applying them.
To me, this is different from actually designing the thing. The objects take shape under my hands, at my direction, but I’m not usually working from some larger picture. I’m just naming things and putting them where they seem to belong.
My, he does wax on, doesn’t he? Do we ever get to the wax off?
Where were we? Oh yes, these two pairs of guys:
function Room:yOverlapCenter(aRoom)
local sp = SegmentPair:forY(self,aRoom)
local lo,hi = sp:overlapLoHi()
return (lo + hi)//2
end
function Room:xOverlapCenter(aRoom)
local sp = SegmentPair:forX(self,aRoom)
local lo,hi = sp:overlapLoHi()
return (lo + hi)//2
end
function Room:xOverlapLength(aRoom)
local sp = SegmentPair:forX(self,aRoom)
local lo,hi = sp:overlapLoHi()
return math.max(hi-lo,0)
end
function Room:yOverlapLength(aRoom)
local sp = SegmentPair:forY(self,aRoom)
local lo,hi = sp:overlapLoHi()
return math.max(hi-lo,0)
end
Let’s do ...Length first. We pick one. This:
function Room:yOverlapLength(aRoom)
local sp = SegmentPair:forY(self,aRoom)
local lo,hi = sp:overlapLoHi()
return math.max(hi-lo,0)
end
We want our SegmentPair to do this for us. So we assume that it can:
function Room:yOverlapLength(aRoom)
local sp = SegmentPair:forY(self,aRoom)
return sp:overlapLength()
end
Mysteriously enough, it can’t. When we run the test:
18: East-West Neighbor has Door information -- Room:172: attempt to call a nil value (method 'overlapLength')
We’re not very surprised. We create the method:
function SegmentPair:overlapLength()
end
We populate it with the code from our old method:
function SegmentPair:overlapLength()
local lo,hi = sp:overlapLoHi()
return math.max(hi-lo,0)
end
We change the reference to sp to self:
function SegmentPair:overlapLength()
local lo,hi = self:overlapLoHi()
return math.max(hi-lo,0)
end
We run the tests and they pass. We look at the other usage:
function Room:xOverlapLength(aRoom)
local sp = SegmentPair:forX(self,aRoom)
local lo,hi = sp:overlapLoHi()
return math.max(hi-lo,0)
end
We change it the same as the other one:
function Room:xOverlapLength(aRoom)
local sp = SegmentPair:forX(self,aRoom)
return sp:overlapLength()
end
This is about as tight as we can get it, unless and until we find a way for the code that uses these not to care X or Y orientation. I hope we’ll get there, but we’re not there now.
We still have these two:
function Room:yOverlapCenter(aRoom)
local sp = SegmentPair:forY(self,aRoom)
local lo,hi = sp:overlapLoHi()~
end
function Room:xOverlapCenter(aRoom)
local sp = SegmentPair:forX(self,aRoom)
local lo,hi = sp:overlapLoHi()
return (lo + hi)//2
end
Feeling strong, we change them both:
function Room:yOverlapCenter(aRoom)
local sp = SegmentPair:forY(self,aRoom)
return sp:overlapCenter()
end
function Room:xOverlapCenter(aRoom)
local sp = SegmentPair:forX(self,aRoom)
return sp:overlapCenter()
end
And we implement:
function SegmentPair:overlapCenter()
local lo,hi = self:overlapLoHi()
return (lo + hi)//2
end
And the tests run. Commit: move overlapCenter and Length to SegmentPair.
Reflection
Now this is no big deal. We reduced two three-line methods to two two-line methods, and created two two-line methods in another class. That’s a net increase in lines of code.
Was it worth it? Well, it was to me, because now all the actual logic of manipulating line segments is centralized in SegmentPair, and it is completely agnostic about X vs Y axis.
And we do have opportunities for more. Let’s look at users of the two x-y-oriented methods again. Here are the only uses of the length one:
function Room:isNeighbor(aRoom)
if self == aRoom then return false end
local doorWidth = 10
return (self:nsCloseEnough(aRoom) and self:xOverlapLength(aRoom)>doorWidth) or
(self:ewCloseEnough(aRoom) and self:yOverlapLength(aRoom)>doorWidth)
end
I’d like to change that code to use SegmentPair. If I can, I can remove the x and y methods from Room. Hm. Let me try something:
function Room:isNeighbor(aRoom)
if self == aRoom then return false end
local doorWidth = 10
local spns = SegmentPair:forX(self,aRoom)
local spew = SegmentPair:forY(self,aRoom)
return (self:nsCloseEnough(aRoom) and spns:overlapLength()>doorWidth) or
(self:ewCloseEnough(aRoom) and spew:overlapLength()>doorWidth)
end
This allows me to remove those two methods entirely. I’d like to do that, burning the bridge behind me, except for the matter of my recent commit. And the matter of a bunch of tests:
_:test("compute x overlap", function()
local s = Room:fromCorners(100,100,200,200)
local n = Room:fromCorners(150,201, 175, 301)
local overlap = s:xOverlapLength(n)
_:expect(overlap).is(25)
n = Room:fromCorners(75,201, 110, 201)
overlap = s:xOverlapLength(n)
_:expect(overlap).is(10)
n = Room:fromCorners(185,201, 250,201)
overlap = s:xOverlapLength(n)
_:expect(overlap).is(15)
n = Room:fromCorners(400,400,450,450)
overlap = s:xOverlapLength(n)
_:expect(overlap).is(0)
end)
_:test("compute y overlap", function()
local w = Room:fromCorners(100,100,200,200)
local e = Room:fromCorners(201,100, 301,150)
local overlap = w:yOverlapLength(e)
_:expect(overlap).is(50)
e = Room:fromCorners(49,175,99,275)
overlap = w:yOverlapLength(e)
_:expect(overlap).is(25)
e = Room:fromCorners(201,225, 250,275)
overlap = w:yOverlapLength(e)
_:expect(overlap).is(0)
e = Room:fromCorners(1,1, 10,10)
overlap = w:yOverlapLength(e)
_:expect(overlap).is(0)
end)
I’m going to do the remove, break the tests, then convert them. They both know what direction they want.
_:test("compute x overlap", function()
local sp
local s = Room:fromCorners(100,100,200,200)
local n = Room:fromCorners(150,201, 175, 301)
local sp = SegmentPair:forX(s,n)
local overlap = sp:overlapLength(n)
_:expect(overlap).is(25)
n = Room:fromCorners(75,201, 110, 201)
local sp = SegmentPair:forX(s,n)
local overlap = sp:overlapLength(n)
_:expect(overlap).is(10)
n = Room:fromCorners(185,201, 250,201)
local sp = SegmentPair:forX(s,n)
local overlap = sp:overlapLength(n)
_:expect(overlap).is(15)
n = Room:fromCorners(400,400,450,450)
local sp = SegmentPair:forX(s,n)
local overlap = sp:overlapLength(n)
_:expect(overlap).is(0)
end)
I converted that by rote. Now for the others. For this one, same thing:
_:test("compute y overlap", function()
local sp
local w = Room:fromCorners(100,100,200,200)
local e = Room:fromCorners(201,100, 301,150)
sp = SegmentPair:forY(w,e)
local overlap = sp:overlapLength(e)
_:expect(overlap).is(50)
e = Room:fromCorners(49,175,99,275)
sp = SegmentPair:forY(w,e)
local overlap = sp:overlapLength(e)
_:expect(overlap).is(25)
e = Room:fromCorners(201,225, 250,275)
sp = SegmentPair:forY(w,e)
local overlap = sp:overlapLength(e)
_:expect(overlap).is(0)
e = Room:fromCorners(1,1, 10,10)
sp = SegmentPair:forY(w,e)
local overlap = sp:overlapLength(e)
_:expect(overlap).is(0)
end)
There’s one more.
_:test("East-West Neighbor has Door information", function()
local r1 = Room:fromCorners(100,100,200,200)
local r2 = Room:fromCorners(201,124, 301, 144)
_:expect(r1:ewCloseEnough(r2)).is(true)
_:expect(r1:yOverlapLength(r2)).is(20)
_:expect(r1:closeEnoughOnWestTo(r2), "not west?").is(true)
local n = Neighbor:fromRooms(r1,r2)
_:expect(n).isnt(nil)
local r1x,r1y = n:getDoor(r1)
_:expect(r1x).is(200)
_:expect(r1y).is(134)
local r2x,r2y = n:getDoor(r2)
_:expect(r2x).is(201)
_:expect(r2y).is(134)
end)
The extra checks here were scaffolding, not really needed. But I’ll just remove the yOverlap one. I could replace it, I suppose. Would that be better? Well, it requires less thinking and is surely safer. We’ll replace it.
_:test("East-West Neighbor has Door information", function()
local r1 = Room:fromCorners(100,100,200,200)
local r2 = Room:fromCorners(201,124, 301, 144)
_:expect(r1:ewCloseEnough(r2)).is(true)
local sp = SegmentPair:forY(r1,r2)
_:expect(sp:overlapLength()).is(20)
_:expect(r1:closeEnoughOnWestTo(r2), "not west?").is(true)
local n = Neighbor:fromRooms(r1,r2)
_:expect(n).isnt(nil)
local r1x,r1y = n:getDoor(r1)
_:expect(r1x).is(200)
_:expect(r1y).is(134)
local r2x,r2y = n:getDoor(r2)
_:expect(r2x).is(201)
_:expect(r2y).is(134)
end)
Now we have completely removed those two methods from Room. Commit: remove Room:x and yOverlapLength.
However, the operational code we changed now looks like this:
function Room:isNeighbor(aRoom)
if self == aRoom then return false end
local doorWidth = 10
local spns = SegmentPair:forX(self,aRoom)
local spew = SegmentPair:forY(self,aRoom)
return (self:nsCloseEnough(aRoom) and spns:overlapLength()>doorWidth) or
(self:ewCloseEnough(aRoom) and spew:overlapLength()>doorWidth)
end
We create two SegmentPair objects, one X and one Y, but we use only one of them. That’s mildly irritating. Why can’t we just create one?
We could break this code out into some nested ifs and get by with one. I do have a more weird idea, but let’s actually do the breakout and see how it looks.
function Room:isNeighbor(aRoom)
if self == aRoom then return false end
local doorWidth = 10
if self:nsCloseEnough(aRoom) then
local spns = SegmentPair:forX(self,aRoom)
return spns:overlapLength()>doorWidth
elseif self:ewCloseEnough(aRoom) then
local spew = SegmentPair:forY(self,aRoom)
return spew:overlapLength()>doorWidth
end
return false
end
That can readily be improved. Let me create some duplication:
function Room:isNeighbor(aRoom)
local sp
if self == aRoom then return false end
local doorWidth = 10
if self:nsCloseEnough(aRoom) then
sp = SegmentPair:forX(self,aRoom)
return sp:overlapLength()>doorWidth
elseif self:ewCloseEnough(aRoom) then
sp = SegmentPair:forY(self,aRoom)
return sp:overlapLength()>doorWidth
end
return false
end
Now the two return lines are the same. Can we remove the duplication? Well, if neither if triggers, we have to return false. Is this too tricky?
function Room:isNeighbor(aRoom)
local sp = nil
if self == aRoom then return false end
local doorWidth = 10
if self:nsCloseEnough(aRoom) then
sp = SegmentPair:forX(self,aRoom)
elseif self:ewCloseEnough(aRoom) then
sp = SegmentPair:forY(self,aRoom)
end
return sp and sp:overlapLength()>doorWidth
end
I explicitly initialized sp to nil to emphasize how the return is checking for sp not having been initialized. In a more sophisticated situation we could init with a FakeSP that always returns some negative value for overlap length, or some such, but that’s not needed here. Dummy objects like that can be really useful, but they are also kind of obscure. Here, I think, we can see what’s up.
Now wouldn’t it be nice if we could close up the nw-ew if statement here? Oh, wait, commit: refactor Room:isNeighbor.
What if, what if … SegmentPair could figure out what kind of SegmentPair is needed, an X one or a Y one, or possibly a nil one, and return it?
That might be asking too much of it. And we’re down to a one-statement if contents, we couldn’t hope to reduce below that inside SegmentPair. Let’s not worry about that right now, but I kind of feel that there’s a pony in here somewhere.
What about that other code, for the Center:
function Neighbor:getDoor(aRoom)
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
local y = aRoom:yOverlapCenter(otherRoom)
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
local x = aRoom:west()
local y = aRoom:yOverlapCenter(otherRoom)
return x,y
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:south()
return x,y
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:north()
return x,y
else
return nil
end
end
Oh this is easy. It’ll go much like the previous one, only double. I had hoped we’d learn from the one before doing the other.
No, on second look, it’s not quite that easy. Hiding in that code is the fact that when we’re east or west of the other room, we return our center in y, and when we’re north or south, we return the center in x. And we have the nil thing. I don’t recall offhand if anyone looks at that.
Ah. No one even uses getDoor yet, we TDD’d it but have not applied it yet in our objects. That’s unusual for me, since I more often call a thing before it even exists, but this time I worked bottom up and we’re not to the top yet. So no help for the nil and I think we may need it. What about the x,y vs y,x thing?
What if our SegmentPair included a handy method to return center coordinate and wall coordinate in the x,y or y,x order, depending on which kind it was?
Or, for now, what if this method did that. Let’s try that first.
Darn. I messed it up, and guess what, I don’t have a good commit to revert to. Fortunately, I only changed one method so I can revert by a paste from here.
Done. Commit: refactored isNeighbor. Ah, looks like I had that revert point already. Anyway, let’s try this again. Smaller steps. Starting from here:
function Neighbor:getDoor(aRoom)
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
local y = aRoom:yOverlapCenter(otherRoom)
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
local x = aRoom:west()
local y = aRoom:yOverlapCenter(otherRoom)
return x,y
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:south()
return x,y
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:north()
return x,y
else
return nil
end
end
The objective is to convert to use SegmentPair, and, hopefully, to wind up able to remove duplication. I’ll do the conversion by rote, causing the code to get marginally worse:
function Neighbor:getDoor(aRoom)
local sp
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
sp = SegmentPair:forY(aRoom,otherRoom)
local y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
local x = aRoom:west()
local y = aRoom:yOverlapCenter(otherRoom)
return x,y
...
I see now what I did wrong last time, I was thinking from Room, not neighbor, and passed self and aRoom into the SegmentPair creation. Should be aRoom, otherRoom as shown. Tests run. Repeat. I’ll try all three. No, I’ll do them one at a time, I’ve already displayed limited ability.
function Neighbor:getDoor(aRoom)
local sp
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
sp = SegmentPair:forY(aRoom,otherRoom)
local y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
local x = aRoom:west()
sp = SegmentPair:forY(aRoom,otherRoom)
local y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:south()
return x,y
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:north()
return x,y
else
return nil
end
end
Tests run. Then:
function Neighbor:getDoor(aRoom)
local sp
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
sp = SegmentPair:forY(aRoom,otherRoom)
local y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
local x = aRoom:west()
sp = SegmentPair:forY(aRoom,otherRoom)
local y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
sp = SegmentPair:forX(aRoom,otherRoom)
local x = sp:overlapCenter()
local y = aRoom:south()
return x,y
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
local x = aRoom:xOverlapCenter(otherRoom)
local y = aRoom:north()
return x,y
else
return nil
end
end
Tests run. Then:
function Neighbor:getDoor(aRoom)
local sp
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
local x = aRoom:east()
sp = SegmentPair:forY(aRoom,otherRoom)
local y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
local x = aRoom:west()
sp = SegmentPair:forY(aRoom,otherRoom)
local y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
sp = SegmentPair:forX(aRoom,otherRoom)
local x = sp:overlapCenter()
local y = aRoom:south()
return x,y
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
sp = SegmentPair:forX(aRoom,otherRoom)
local x = sp:overlapCenter()
local y = aRoom:north()
return x,y
else
return nil
end
end
Tests run. We should be able to remove the x and y methods, perhaps breaking some tests.
And no tests break, they’re not using that. Because really hardly anyone does. They do check the getDoor function, I hope. We’ll try to remember to look. Now can we tidy up this code a bit?
First I’ll remove the redundant mentions of local, they’re in the way.
Now I’d like to reorder each if branch to be more similar, just to aid the eyes.
function Neighbor:getDoor(aRoom)
local sp
local x,y
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
sp = SegmentPair:forY(aRoom,otherRoom)
x = aRoom:east()
y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
sp = SegmentPair:forY(aRoom,otherRoom)
x = aRoom:west()
y = sp:overlapCenter()
return x,y
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
sp = SegmentPair:forX(aRoom,otherRoom)
x = sp:overlapCenter()
y = aRoom:south()
return x,y
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
sp = SegmentPair:forX(aRoom,otherRoom)
x = sp:overlapCenter()
y = aRoom:north()
return x,y
else
return nil
end
end
Now I want to rename x and y to center and wall and return the appropriate ordering:
function Neighbor:getDoor(aRoom)
local sp
local x,y
local wall,center
local otherRoom = self:otherRoom(aRoom)
if aRoom:closeEnoughOnWestTo(otherRoom) then
sp = SegmentPair:forY(aRoom,otherRoom)
wall = aRoom:east()
center = sp:overlapCenter()
return wall,center
elseif aRoom:closeEnoughOnEastTo(otherRoom) then
sp = SegmentPair:forY(aRoom,otherRoom)
wall = aRoom:west()
center = sp:overlapCenter()
return wall,center
elseif aRoom:closeEnoughOnNorthTo(otherRoom) then
sp = SegmentPair:forX(aRoom,otherRoom)
center = sp:overlapCenter()
wall = aRoom:south()
return center,wall
elseif aRoom:closeEnoughOnSouthTo(otherRoom) then
sp = SegmentPair:forX(aRoom,otherRoom)
center = sp:overlapCenter()
wall = aRoom:north()
return center,wall
else
return nil
end
end
Note how this seemingly silly change brings out the fact that the EW ones and NW ones are returning the results transposed one from the other. We should be able to make use of that.
We have duplication in the creation of the SegmentPair and the call to it. And we have that irritating requirement to return a nil if the rooms aren’t adjacent.
It’s worth noting that this method isn’t in use other than in tests, who check to see that we’re getting the coordinates right. What it does is return an x,y pair. What it says it that it’s going to return a Door. We don’t even have a door object yet. This means that we have some flexibility here, since no one but tests even knows this exists. On the other hand, if we do change what it does, we should do that in the light of what the actual program needs. All we have right now is a glimmer in my eye about drawing spots on the map where the doors are.
Our tests are green. Commit: refactoring Neighbor:getDoor().
I think I’m tired of thinking. Let’s pause, sum up, and maybe come back this afternoon or tomorrow with another article.
Summary
After another attempt to express how smoothly we can incrementally move functionality from one object to another, increasing generality along the way, we actually did a little more of that, resulting in the complete removal of four more methods from Room, mostly by changing Neighbor to reference a SegmentPair rather than Room.
One nice thing about that that I haven’t mentioned is that SegmentPair is a rather simple dedicated object. It’s immutable, which always makes things simpler, and it handles twice the capability of the code that we previously had, because it deals with both dimensions X and Y rather than just one or the other.
It’s generally good when we’re able to pull out a small simple object and use it instead of a larger one. Rooms should be about being rooms, not about geometry. So it makes sense to delegate geometry to SegmentPair.
However, there’s a related kind of code that should be avoided. The pattern is called Primitive Obsession, and it’s not a good pattern. We could imagine writing all the code that gets centers and such by fetching out the x’s and y’s and w’s and h’s and manipulating them wherever we needed info. The related pattern, Feature Envy, tells us that when we are fetching values from object X and manipulating them, we are getting a clue about something that X ought to do for us. But when those values are primitives like numbers, and often even slightly more abstract values like vectors, the fact that these primitive types don’t know what they represent can lead to trouble.
So while we do push toward using simpler objects, we still do best when they are abstractions that make sense in our problem space and if they’re too much like simple numbers, we consider that a concern.
Here, I think we’ve done well. I don’t promise to believe that forever, but I believe it now.
Overall, the past two days have gone smoothly, with a new feature, the door, coming into view through the fog, and the code drifting smoothly from places where it seems not to belong over to places where it does belong.
I remain concerned about the NS vs EW separation that seems necessary for checking adjacency, and I admit that I’m going to draw some pictures and think about that. If I get anywhere, you’ll hear about it.
Until then, be well, and I’ll see you next time.