Some thoughts on consistency, clarity, abstraction, and efficiency. A LOVELY REFACTORING!

I woke up early today, so I’m all fresh and ready to go at 0630 on the dot. As I was showering, I was thinking about the doors we’re working on. I’ve just about decided to have some kind of object that holds the necessary information to position the door. I had been thinking that perhaps it would be a rectangle kind of thing, with x1,x2, y1,y2, you know the drill. And I reflected that this program draws the rooms in CENter mode, which is to say that we specify the room’s center and its width and height, and it’s drawn accordingly. So there I was with a door where (I thought) I wanted corners, and rooms, where I have center.

And that led me to think about this.

Room has a method corners(). It’s a function. In the case of room, it does a very simple calculation and returns four values in a table:

function Room:corners()
    local hw = self:halfWidth()
    local hh = self:halfHeight()
    return {self.x - hw, self.y - hh, self.x + hw, self.y + hh}
end

(I think we should look deeper at that. I’m not sure anyone uses that information as a table. It might be easier all around if we just return the four values.)

However, when I get the room’s center values, I refer to them directly, as room.x and room.y. What’s up with that? Shouldn’t it be one way or the other?

And then, if I make this door rectangle thing, and it knows its corners, what if I want the center of the door? And in fact, I think that’s all we want, and from the door design viewpoint, this discussion is moot. But from a design viewpoint, we’d have

Room
center accessed as member variables, x,y; corners accessed as function corners().
Door
center accessed as function center(); corners (could be) accessed as member variables x1,y1,x2,y2.

We’d have two similar ideas implemented in two different, opposite ways. That would be confusing. How “should” we deal with these matters?

Offhand, I see at least these forces acting.

Consistency
Similar things should be done similar ways.
Clarity
Things should simply express what the code needs to express.
Abstraction
Things should express what they are, more than how they work.
Efficiency
Things should execute as little code as possible.

In Smalltalk, everything is a method. Everything you do comes down to a function call. If you fetch a member variable with a call to myRoom getX, getX is a method, a function. If you calculate the square root of x, you use a function x sqrt. You cannot tell, without reading the code whether the method you’re calling just fetches the answer or computes it.

And that’s a good thing on at least the first three of the forces above. Everything is done consistently, there’s no additional decorations like parens or dots versus colons, and since you can’t tell by looking, the implementor of your object is free to change its internals as needed. If we take a lot of square roots and we have to send out in the mail to get them computed, then x might cache its square root and no one has to change their code.

And that … would improve efficiency.

A simple assumption might be to avoid method calls if you can do direct accesses, because surely a function call is more costly than no function call. But that may not be the right decision to make.

In Space Invaders 79 I wrote a Codea program to measure the cost of a method call, with this result:

So ten million calls cost us 0.44 seconds, or, if I’ve done this right, less than 50 nanoseconds per method call. So our five calls every 60th (or 120th) of a second cost us about 250 microseconds. I think the cost argument doesn’t hold water.

No one has corrected my arithmetic yet, so I’m going with it. The cost of a single method call is close to zero. We do still have to be concerned if we’‘re doing a zillion in a tight loop, but probably not otherwise.

Kent Beck advised us to optimize code for speed only in the presence of a performance test showing that it needs optimization. Otherwise we should optimize for clarity.

I’d say, looking at the forces above, that using method calls for everything will optimize for consistency and clarity, and will automatically add a useful layer of abstraction, while not impacting efficiency negatively enough to matter.

I’ve been moving in that direction slowly with Codea. Since you have the member variables right there, it’s so easy to just grab them. But slowly, I’ve started covering even constants with a method, as with this one:

function Room:closeEnough()
    return 5
end

We surely do need the idea of when a room is close enough to another room to have a door between them. But we could just as well have put this into a Constants object or a global. But I’ve been leaning to the method style for a while, kind of coming onto a slightly different course.

I’m always watching how I work, what my actual practices are compared to my ideals, and how both the ideals and actuals change. It’s part of what makes programming enjoyable to me.

I commend the same attitude to you.

Now let’s do some actual work.

More Door Stuff

We left off working on doors with this test green:

        _:test("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:yOverlap(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)
        end)

What we’re working toward is for the Neighbor object to be able to return a door position for each of the two rooms it holds. In doing that, we’ve broken out some of the formerly combined “ns” and “ew” decisions, to give us access to the details. Formerly we were answering the question “is the other room close enough and aligned well enough”, and now we want to remember the details.

The getDoor function computes everything anew, if I recall:

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
    else
        return nil
    end
end

My plan is to write all this out sequentially, and when it’s done, see what we should do to consolidate things and avoid duplication of code and of calculation. In the code above, we have already computed close enough and overlap, so this is redundant. But we didn’t save the information. We weren’t even in a position to save it.

We’ll get this to work, and then we’ll get it right.

I think we can just create some more tests and make them work. I’m also wondering what happens if we drive the existing test “backwards”. Presently it’s written to check whether r1 west of r2 lets us compute r1’s door, and it does. What would happen if we asked the question the other way around. Let’s code that test in. I think this is it:

            local r2x,r2y = n:getDoor(r2)
            _:expect(r1x).is(201)
            _:expect(r2y).is(134)

This shouldn’t run. But the result surprises me a bit. Oh. I wrote r1. Here’s what I meant:

            local r2x,r2y = n:getDoor(r2)
            _:expect(r2x).is(201)
            _:expect(r2y).is(134)

Now I get the result I expect:

18: Neighbor has Door information  -- Actual: nil, Expected: 201
18: Neighbor has Door information  -- Actual: nil, Expected: 134

It seems that I should be able to essentially copy what we have:

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
    else
        return nil
    end
end

We add another clause:

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
    else
        return nil
    end
end

Delightfully, that actually works. Now we can do north-south and see where we stand. But first, commit: Neighbor calculates east and west doors correctly.

Moving Right Along

It’s 0737. The code above took moments. The dissertation prior to that took up most of the time. Now to create a north-south test. What do you bet I get the numbers wrong?

        _:test("North-South Neighbor has Door information", function()
            local r1 = Room:fromCorners(100,100,200,100)
            local r2 = Room:fromCorners(125, 50, 145, 98) -- south and smaller
            local n = Neighbor:fromRooms(r1,r2)
            local r1x,ry1 = n:getDoor(r1)
            _:expect(r1x).is(135)
            _:expect(r1y).is(100)
        end

I just did the one side for now. Of course, now I have to figure out which half of the N-S function gives me the right answer. Let’s see. r1 is north of r2. Maybe that helps. Or maybe I could just do them both?

        _:test("North-South Neighbor has Door information", function()
            local r1 = Room:fromCorners(100,100,200,100)
            local r2 = Room:fromCorners(125, 50, 145, 98) -- south and smaller
            local n = Neighbor:fromRooms(r1,r2)
            local r1x,r1y = n:getDoor(r1)
            _:expect(r1x).is(135)
            _:expect(r1y).is(100)
            local r2x,r2y = n:getDoor(r2)
            _:expect(r2x).is(135)
            _:expect(r2y).is(98)
        end)

That also let me fix two typos. Did you see them? I bet you didn’t. Neither did I, the first time.

So this test should fail four times. And it does.

19: North-South Neighbor has Door information  -- Actual: nil, Expected: 135
19: North-South Neighbor has Door information  -- Actual: nil, Expected: 100
19: North-South Neighbor has Door information  -- Actual: nil, Expected: 135
19: North-South Neighbor has Door information  -- Actual: nil, Expected: 98

Now to fix getDoor:

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

This discovers that we apparently haven’t done xOverlapCenter yet. Could be, let’s look. True enough. Here’s the y version:

function Room:yOverlapCenter(aRoom)
    xx,sLo,xx,sHi = unpack(self:corners())
    xx,rLo,xx,rHi = unpack(aRoom:corners())
    local lowY = max(sLo,rLo)
    local highY = min(sHi,rHi)
    return (lowY + highY)//2
end

So the x might be:

function Room:xOverlapCenter(aRoom)
    sLo,yy,sHi,yy = unpack(self:corners())
    rLo,yy,rHi,yy = unpack(self:corners())
    local lowX = max(sLo,rLo)
    local highX = min(sHi,rHi)
    return (lowX + highX)//2
end

If this makes the test run, I’ll consider it a miracle. And no, not quite:

19: North-South Neighbor has Door information  -- Actual: 150.0, Expected: 135

Hm. That’s the first one, and the second worked. Check the test:

        _:test("North-South Neighbor has Door information", function()
            local r1 = Room:fromCorners(100,100,200,100)
            local r2 = Room:fromCorners(125, 50, 145, 98) -- south and smaller
            local n = Neighbor:fromRooms(r1,r2)
            local r1x,r1y = n:getDoor(r1)
            _:expect(r1x).is(135)
            _:expect(r1y).is(100)
            local r2x,r2y = n:getDoor(r2)
            _:expect(r2x).is(135)
            _:expect(r2y).is(98)
        end)

That sure looks right. Why is the one way right and the other wrong. And the y’s are right. r1 is to the north. And the new code? Ha:

function Room:xOverlapCenter(aRoom)
    sLo,yy,sHi,yy = unpack(self:corners())
    rLo,yy,rHi,yy = unpack(self:corners())
    local lowX = max(sLo,rLo)
    local highX = min(sHi,rHi)
    return (lowX + highX)//2
end

Second line should be aRoom.

function Room:xOverlapCenter(aRoom)
    sLo,yy,sHi,yy = unpack(self:corners())
    rLo,yy,rHi,yy = unpack(aRoom:corners())
    local lowX = max(sLo,rLo)
    local highX = min(sHi,rHi)
    return (lowX + highX)//2
end

And the tests are green. I’m declaring victory.

Commit: Neighbor calculates all door positions.

Let’s reflect briefly on what we have and how we got here, then decide what to do next.

Reflection

Now the way I recall it, I was smart enough to break apart the combined methods ewCloseEnough() and nsCloseEnough() into closeEnoughOnEastTo() and so on. That let me use those functions individually in the Neighbor class, who needs to know which side of the room is adjacent, so he can return the near wall or far wall’s coordinate.

Those broke out fairly nicely, though I seem to recall that I got them wrong at least once, by copying and pasting the wrong side of the return statement. This “encouraged” me to write a few more tests, and that helped get the code right and helped build up my confidence.

I find that confidence is important. When I start to get that nagging feeling that this might not be right, I begin to feel tense, I’m more easily distracted away from what I’m doing to worry about what I’ve done or am about to do … and the result is that I make more mistakes.

Even if I didn’t make more mistakes, I don’t like feeling fearful about my code, and I don’t like the longer sessions of debugging that arise when tests lag behind code, so all in all, I do better with tests.

And you have seen me, again and again, proceed without them. I hope you’ve also noticed that I get in trouble far more often when I go without them. So if all I can do is serve as a bad example, I’m OK with it.

But sometimes I hope I get to show a good example, and this little process on the Neighbor knowing door positions has mostly gone well.

I’ve committed the code. If you’ll permit, it’s now about 0810 and I think I’ll go pick up a chai.

Now then …

It’s 0845, pleasant top-down drive to the *$. Last warm day perhaps: we’ll see.

What now?

We have redundancy in creating and computing with our Neighbors. Originally, we just used them to compute the possible paths between rooms: you may recall that the class was named “Line” for a while. Neighbors have grown up a bit since then, and they know whether they are on a primary path between rooms, that is. a part of the original spanning tree, or whether they are “secondary”, representing a loop in the room network.

We may want to retain that information. One possibility is that we’ll just keep all the neighbors connected. But knowing that we can close off some doors and keep the maze connected could be useful. Maybe there are doors that close off a path just as the monsters attack, leaving you still able to move around, but hampered in which way you can go. Who knows: the info is useful, and I think we’ll keep it. I do think it should be kept more private, somewhere inside the Neighbor class.

And we want to provide each Room with a list of its neighbors. We need that in order to draw the room’s doors in the right places, and we need it to manage the players’s transition from room to room. That’s new functionality that we’ve not provided.

Right now we’re not doing anything clever about optimizing the neighbor searching. We check each room against all other rooms to see whether they are neighbors. And then when a room asks a door where to put the door, we compute most of that information again. It seems that we shouldn’t duplicate that effort.

There is a slight argument for computing the door positions every time they’re asked for. There might be game play where a room vanishes and appears, or moves around. That could make it desirable for the calculation to be dynamic. But despite thinking it could be a cool feature, I don’t think we know enough to cater for it now. We’d have to fuss with the room’s list of rooms and so on anyway. So that doesn’t convince me that I simply must compute the information when it’s asked for as it is now. That said, the question is rarely asked and it’s never going to be asked a zillion times after the initial map creation is complete. It’ll be asked a few times, once for each door in the current room. So I cannot choose the cup in front of me, er I mean so I don’t really mind that it’s calculated on the fly.

What bothers me more is that it’s calculated in two different places, in slightly different ways.

When we decide whether a room is a neighbor, we do like this:

...
    Rooms[1]:colorNeighborsIn(Rooms, color(255,0,0,100))
...

function Room:colorNeighborsIn(rooms, aColor)
    self:setColor(aColor)
    for i,r in ipairs(rooms) do
        if r:isNeighbor(self) then
            if r:getColor() ~= aColor then
                self:insertNewNeighbor(Neighbors, Neighbor:primary(self, r))
                r:colorNeighborsIn(rooms, aColor)
            else
                self:insertNewNeighbor(Neighbors, Neighbor:secondary(self, r))
            end
        end
    end
end


function Room:isNeighbor(aRoom)
    if self == aRoom then return false end
    local doorWidth = 10
    return (self:nsCloseEnough(aRoom) and self:xOverlap(aRoom)>doorWidth) or
           (self:ewCloseEnough(aRoom) and self:yOverlap(aRoom)>doorWidth)
end

That isNeighbor method returns a boolean based on whether the rooms are close enough together. Our new method duplicates that thinking substantially:

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 closeEnough methods here are the ones that the ew and ns ones call:

function Room:ewCloseEnough(aRoom)
    return self:closeEnoughOnEastTo(aRoom) or self:closeEnoughOnWestTo(aRoom)
end

So that’s entirely duplicated. And the overlap methods are quite similar:

function Room:xOverlap(aRoom)
    sLo,xx,sHi,xx = unpack(self:corners())
    rLo,xx,rHi,xx = unpack(aRoom:corners())
    return max(min(sHi,rHi)-max(sLo,rLo),0)
end

function Room:xOverlapCenter(aRoom)
    sLo,yy,sHi,yy = unpack(self:corners())
    rLo,yy,rHi,yy = unpack(aRoom:corners())
    local lowX = max(sLo,rLo)
    local highX = min(sHi,rHi)
    return (lowX + highX)//2
end

So essentially everything here is duplicated, partly in actual code text, and partly by being called more than once.

We can, however, remove this textual duplication fairly readily. How about if we do this:

function Room:xOverlapPoints(aRoom)
    sxLo, _y, sxHi, _y = unpack(self:corners())
    rxLo, _y, rxHi, _y = unpack(aRoom:corners())
    return sxLo, sxHi, rxLo, rxHi
end

Now that function can be used in both of these functions:

function Room:xOverlap(aRoom)
    sLo,sHi,rLo,rHi = self:xOverlapPoints(aRoom)
    return max(min(sHi,rHi)-max(sLo,rLo),0)
end

function Room:xOverlapCenter(aRoom)
    sLo,sHi,rLo,rHi = self:xOverlapPoints(aRoom)
    local lowX = max(sLo,rLo)
    local highX = min(sHi,rHi)
    return (lowX + highX)//2
end

We now notice some more duplication. Both the lower functions really only want the min and max of the same pairs. We can make our upper function return those instead:

Hm, I’ve messed this up three times in a row. I need a commit here before I go in again: refactor xOverlapPoints from x overlap functions.

OK, I guess I have to do this in both those functions at once. No, not if I create a new method doing my max/min thing:

function Room:xOverlapLoHi(aRoom)
    local sxLo,sxHi,rxLo,rxHi = self:xOverlapPoints(aRoom)
    return max(sxLo,rxLo),min(sxHi,rxHi)
end

Now I can use this in one place at a time, leaving the other one working. Then this works:

function Room:xOverlap(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return max(hi-lo,0)
end

And we can then do this:

function Room:xOverlapCenter(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return (lo + hi)//2
end

And our tests still run.

I want to comment on what I just did, and then we’ll look at this and unwind it a bit.

I tried three or more times to create that little “LoHi” method by editing the Points method in place and then applying it in the two methods that needed it. Every time I messed something up. The Lo and Hi and max and min are tricky, since the low is the max of the lows and the high the min of the highs. Anyway, didn’t work.

So I created a new method that returned the new pair of values. That allowed me to put it in one place, run my tests, put it in the other, and run them again. And this time they worked. Could be fourth time lucky, but I think it was “smaller steps lucky”.

Now we can unwind this if we want to:

function Room:xOverlapLoHi(aRoom)
    local sxLo,sxHi,rxLo,rxHi = self:xOverlapPoints(aRoom)
    return max(sxLo,rxLo),min(sxHi,rxHi)
end

function Room:xOverlapPoints(aRoom)
    sxLo, _y, sxHi, _y = unpack(self:corners())
    rxLo, _y, rxHi, _y = unpack(aRoom:corners())
    return sxLo, sxHi, rxLo, rxHi
end

I don’t know, though. That looks OK to me. The one guy is returning some point things, the other guy the processed max min things. The names aren’t quite right, but the two steps are rather distinct.

What if we try to make the names less obscure. The “Points” aren’t really points, are they? They are the low and high (x) values for the adjacent walls. Let’s start by naming that function xOverlapValues. It might be a small improvement.

function Room:xOverlapValues(aRoom)
    sxLo, _y, sxHi, _y = unpack(self:corners())
    rxLo, _y, rxHi, _y = unpack(aRoom:corners())
    return sxLo, sxHi, rxLo, rxHi
end

Now what about the names? And by the way, those should be local. I’m glad I looked at this.

function Room:xOverlapValues(aRoom)
    local sxLo, _y, sxHi, _y = unpack(self:corners())
    local rxLo, _y, rxHi, _y = unpack(aRoom:corners())
    return sxLo, sxHi, rxLo, rxHi
end

In my mind, s stands for self here and r for room. Not obvious. But now I’m wondering whether we’re returning quite the right things at all. Our users, all two of them, want two values back from this function and the LoHi one, namely the low X value of the overlap and the high X value of the overlap. The low X is the max of the two low ends, the high X is the min of the two high ends. (Yes, weird. Think about it.)

We shouldn’t be unpacking the corners at all (and possibly corners should return unpacked). I’ve thought that thought before. Let’s examine all users of corners:

Everyone who uses corners unpacks it, except this guy:

function Room:intersects(aRoom)
    return self:intersectCorners(self:corners(), aRoom:corners())
end

And the function he calls unpacks:

function Room:intersectCorners(corners1, corners2)
    x1lo, y1lo, x1hi, y1hi = unpack(corners1)
    x2lo, y2lo, x2hi, y2hi = unpack(corners2)
    if y1lo > y2hi or y2lo > y1hi then return false end
    if x1lo > x2hi or x2lo > x1hi then return false end
    return true
end

Let’s change it now, since we’re on a green bar. A quick commit: refactoring x overlap logic.

Now then:

function Room:intersects(aRoom)
    return self:intersectCorners({self:corners()}, {aRoom:corners()})
end

function Room:corners()
    local hw = self:halfWidth()
    local hh = self:halfHeight()
    return self.x - hw, self.y - hh, self.x + hw, self.y + hh
end

function Room:intersectCorners(corners1, corners2)
    x1lo, y1lo, x1hi, y1hi = unpack(corners1)
    x2lo, y2lo, x2hi, y2hi = unpack(corners2)
    if y1lo > y2hi or y2lo > y1hi then return false end
    if x1lo > x2hi or x2lo > x1hi then return false end
    return true
end

Everywhere else, I just removed the unpack. The thing here was the intersects puts corners into tables, as opposed to putting baby in a corner, and the intersectCorners still unpacks. Tests run. Commit: corners returns tuple not table.

I have a feeling some of this will get nicer. Where were we? Oh, yes, the names in here:

function Room:xOverlapLoHi(aRoom)
    local sxLo,sxHi,rxLo,rxHi = self:xOverlapValues(aRoom)
    return max(sxLo,rxLo),min(sxHi,rxHi)
end

function Room:xOverlapValues(aRoom)
    local sxLo, _y, sxHi, _y = self:corners()
    local rxLo, _y, rxHi, _y = aRoom:corners()
    return sxLo, sxHi, rxLo, rxHi
end

In this xOverlapValues function, we aren’t even interested in the y values. We dump them all into a trash variable _y. We want something like this:

function Room:xOverlapValues(room)
    selfLoX, selfHiX = self:xValues()
    roomLoX, roomHiX = room:xValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

Let’s write that:

function Room:xValues()
    local xyxy = {self:corners()}
    return xyxy[1],xyxy[3]
end

So that works just fine and it’s rather more clear, I think. Now for the other guy with the cryptic names:

function Room:xOverlapLoHi(aRoom)
    local sxLo,sxHi,rxLo,rxHi = self:xOverlapValues(aRoom)
    return max(sxLo,rxLo),min(sxHi,rxHi)
end

We can do him the same way and see if we like it:

function Room:xOverlapLoHi(room)
    local selfLox,selfHiX,roomLox,roomHiX = self:xOverlapValues(room)
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

Tests still green. I should commit more often. “Refactoring x values in Room”

Now the fact is, most of these functions only have one caller. Their sole purpose is to unwind the data structure we have into a data structure that we want for our current purpose. They are what we might call, after Beck, “explaining methods”.

All this code could be written in line, and many of my readers might well write it in line. However, had anyone written all this in line, they’d likely have left it alone the moment that it worked. If it was a bit of a pain for the actual functions that use the code, too bad, we don’t dare mess with it, even though we have tests. It’s just so messy we can’t see our way out.

But the thing is, there’s usually a way out, inch by inch, step by step, slowly we refactor … and along the way we discover ways of expressing what we’re doing that will, we hope, help us when we read this code again someday.

What about us grils?

But this is all about x. What about y? Don’t we have a very similar, symmetric opportunity to improve the y code? It seems certain that we have. And I have a feeling, just an inkling, that after we do that we might find some more consolidation, because the y stuff will look a lot like the x stuff.

Let’s find out. Why not?

Y, not?

We came down, ultimately, to these methods for the x room info:

function Room:xOverlap(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return max(hi-lo,0)
end

function Room:xOverlapCenter(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return (lo + hi)//2
end

The corresponding methods for y look like this:

function Room:yOverlap(aRoom)
    xx,sLo,xx,sHi = self:corners()
    xx,rLo,xx,rHi = aRoom:corners()
    return max(min(sHi,rHi)-max(sLo,rLo),0)
end

function Room:yOverlapCenter(aRoom)
    xx,sLo,xx,sHi = self:corners()
    xx,rLo,xx,rHi = aRoom:corners()
    local lowY = max(sLo,rLo)
    local highY = min(sHi,rHi)
    return (lowY + highY)//2
end

This time we probably don’t need to think quite so hard, we can copy what we did. We could copy/paste our way to success, but I’m more inclined to browse and retype. Doing so engages a bit more of my mind, and I like to understand what I’m doing.1

So we’ll posit that we can write the methods like this. I’ll go one at a time. I’m a programmer, Jim, not a juggler.

function Room:yOverlap(aRoom)
    local lo,hi = self:yOverlapLoHi(aRoom)
    return max(hi-lo,0)
end

This demands yOverlapLoHi, which ought to look like this:

Oh. I’ve done something wrong. I don’t know what, and I don’t care, but I do see a bug. First revert.

The problem I spotted when I went to copy xOverlapLoHi was this:

function Room:xOverlapLoHi(room)
    local selfLox,selfHiX,roomLox,roomHiX = self:xOverlapValues(room)
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

The first selfLox should be selfLoX. What I wonder is why no tests are failing. That should have errored somewhere, with

attempt to compare nil with number

Here we have what Pirsig, in The Art of Motorcycle Maintenance, called a “gumption trap”. You know when you’re removing a screw and munge up the slot so that you can’t remove it. All the wind goes out of your sails, all the gumption goes out of you gut or wherever gumption is stored.

This error stalls me. I see the error and the fix, but the fact that nothing has broken tells me that my tests are somehow inadequate, or the code isn’t doing what I think it’s doing.

The method xOverlap uses this function. I don’t want to but I feel the need to find out why it’s not failing some test, indeed all tests, unless somehow xOverlap isn’t called.

Shouldn’t every call to xOverlap have failed?

Ah, I see it. This:

function Room:xOverlapValues(room)
    selfLoX, selfHiX = self:xValues()
    roomLoX, roomHiX = room:xValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

That’s defining those variables as global, so the other function sees a value that it shouldn’t see, but one that is close enough, probably actually correct. So if we fix that, we should get test failures.

And we do. Now to fix this:

function Room:xOverlapLoHi(room)
    local selfLoX,selfHiX,roomLox,roomHiX = self:xOverlapValues(room)
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

And we still get an error because I also did roomLox wrong. Time for a break, I reckon. Anyway:

function Room:xOverlapLoHi(room)
    local selfLoX,selfHiX,roomLoX,roomHiX = self:xOverlapValues(room)
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

Tests good: commit fixed accidental globals in xOverlapValues.

We are actually up to 1040, which makes this a particularly long session by my standards. But I’d like to get the y’s done to match the x’s, so I’ll press on unless something horrid happens.

Back to it.

We do this like the x version:

function Room:yOverlap(aRoom)
    local lo,hi = self:yOverlapLoHi(aRoom)
    return max(hi-lo,0)
end

This demands:

function Room:yOverlapLoHi(room)
    local selfLoY,selfHiY,roomLoY,roomHiY = self:yOverlapValues(room)
    return max(selfLoY,roomLoY),min(selfHiY,roomHiY)
end

I note this is exactly the same as the x version except for substituting some y’s for x’s. That’s interesting.

The code now demands yOverlapValues:

function Room:yOverlapValues(room)
    local selfLoX, selfHiX = self:yValues()
    local roomLoX, roomHiX = room:yValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

Here, I made a point of calling yValues instead of xValues but left the other names alone. I expect to deal with that in a moment. I’m trying to produce even more obvious duplication.

Now the code demands the function yValues, which we are happy to provide:

function Room:yValues()
    local xyxy = {self:corners()}
    return xyxy[2],xyxy[4]
end

That’s interesting, isn’t it?

Our tests are running again. Commit: refactoring y overlap to be more like x. Now there’s that other function that doesn’t match:

function Room:xOverlapCenter(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return (lo + hi)//2
end

function Room:yOverlapCenter(aRoom)
    xx,sLo,xx,sHi = self:corners()
    xx,rLo,xx,rHi = aRoom:corners()
    local lowY = max(sLo,rLo)
    local highY = min(sHi,rHi)
    return (lowY + highY)//2
end

We should be able to just type that in:

function Room:yOverlapCenter(aRoom)
    local lo,hi = self:yOverlapLoHi(aRoom)
    return (lo + hi)//2
end

I expect my tests to run. Mysteriously, they do.

Commit: yOverlap and xOverlap done same way. Duplication exists.

Duplication sure does exist. Look at these pairs:

function Room:xOverlapCenter(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return (lo + hi)//2
end

function Room:yOverlapCenter(aRoom)
    local lo,hi = self:yOverlapLoHi(aRoom)
    return (lo + hi)//2
end

… and …

function Room:xOverlapValues(room)
    local selfLoX, selfHiX = self:xValues()
    local roomLoX, roomHiX = room:xValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

function Room:yOverlapValues(room)
    local selfLoX, selfHiX = self:yValues()
    local roomLoX, roomHiX = room:yValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

… and …

function Room:xOverlapLoHi(room)
    local selfLoX,selfHiX,roomLoX,roomHiX = self:xOverlapValues(room)
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

function Room:yOverlapLoHi(room)
    local selfLoY,selfHiY,roomLoY,roomHiY = self:yOverlapValues(room)
    return max(selfLoY,roomLoY),min(selfHiY,roomHiY)
end

… and …

function Room:xOverlap(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return max(hi-lo,0)
end

function Room:yOverlap(aRoom)
    local lo,hi = self:yOverlapLoHi(aRoom)
    return max(hi-lo,0)
end

… and even ..

function Room:xValues()
    local xyxy = {self:corners()}
    return xyxy[1],xyxy[3]
end

function Room:yValues()
    local xyxy = {self:corners()}
    return xyxy[2],xyxy[4]
end

It looks as if, if we could just deal with 1,3 vs 2,4, all this code would collapse down to the same in x and y.

That is quite interesting.

Before I attack that, I’m going to take a well-deserved break, to recharge my battery and the iPad’s.

1104: break.

Later that day …

At 1345, to be more precise, I’m ready to see what can be done about collapsing all this nearly duplicated code.

The interesting and somewhat strange thing about this nest is that the two branches are really totally the same, all the way down until one of them returns items 1 and 3 from an array, and one returns items 2 and 4. To make that happen, everything above is named x this and y that so as to alow the bottom level to be broken out:

function Room:xValues()
    local xyxy = {self:corners()}
    return xyxy[1],xyxy[3]
end

function Room:yValues()
    local xyxy = {self:corners()}
    return xyxy[2],xyxy[4]
end

I had originally, that is, while I was sitting in the other room reading a Bobiverse novel, I had originally thought of passing some kind of flag or function down, to trigger the bottom function to fetch the right values. That would be interesting but a bit off, I think.

A thing to keep in mind, as well, is that we actually call either xValues or yValues twice, once for self and once for the other room. In a way, that argues for passing the single flag or function.

An alternative might be to fetch either the x’s or the y’s right at the top and then pass them down to the functions that want them.

Another thing I was thinking of was to drop the use of the xOverlap and yOverlap functions in detecting whether the rooms are neighbors, perhaps instead asking for the door coordinates and having that return some fake or nil value if there was no room for a door. We’d still have to check, though, because rooms can overlap, but not enough to leave room for a door. So I think that’s not in the cards.

While I’m thinking of it, I think those two functions should be named x and yOverlapAmount or OverlapLength. Length will do.

That goes well. Commit: rename xand yOverlap to OverlapLength.

Now back to the issue at hand, combining some of this duplicate code.

Maybe it’s worth observing that we are, in essence, working with simple segments of a single line, answering whether, they um overlap, how much they overlap, and where the center of that overlap is. It’s possible that a helper object would be just the thing here.

Suppose we had this thing, a Segment comparator or something, and it had two ways of being created, one from a horizontal line and one from a vertical. Then our two methods (or perhaps still more than two) could just create one of these comparators, and call it. All the inner methods that we presently have as duplicates would just have one form inside the comparator.

I think I like this idea, and I have an idea how to create it. Let me describe the plan. This will be a use of Chet Hendrickson’s favorite object-oriented trick, the double dispatch.

In our case here, we’ll proceed incrementally after building up the base comparator, which will basically be managing four line coordinates, two for self and two for another Room. We’ll call from our top level method directly to the comparator, and at first, it will just call back, falling through all the code we now have. Then we’ll move that code down to the comparator, bit by bit, consolidating, until we’re done.

I think this is going to work. If not, well, I have a nice commit point right here.

First the new class. I’ll call it SegmentPair, because that’s what it has in it.

For now, I don’t know how it will really initialize itself. Instead, I’ll have two factory methods, one for x and one for y, each taking the two rooms of interest. Here goes. I’ll start with x.

-- SegmentPair
-- RJ 20201110

SegmentPair = class()

-- class methods

function SegmentPair:forX(room1,room2)
    return SegmentPair(room1,room2,true)
end

function SegmentPair:init(room1, room2, isX)
    self.room1 = room1
    self.room2 = room2
    self.isX = isX
end

I am frankly guessing at what I’ll need, but whatever it is, it’l all be inside. Now let’s see if someone can use this thing. We could start at the bottom of the calling chain, or the top. I certainly want to wind up at the top, so let’s see if we can do this and make it work. I’ll start with xOverlapLength, pretty arbitrarily.

Presently that is this:

function Room:xOverlapLength(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return max(hi-lo,0)
end

We replace that with use of the SegmentPair:

function Room:xOverlapLength(aRoom)
    local sp = SegmentPair:forX(self,aRoom)
    local lo,hi = sp:overlapLoHi()
    return max(hi-lo,0)
end

This demands sp:overlapLoHi() which we implement thus:

function SegmentPair:overlapLoHi()
    return self.room1:xOverlapLoHi(self.room2)
end

We just call right back. We know we’re doing X and for now we don’t even care. We’d like not to call back, we’d like to implement our own version of this:

function Room:xOverlapLoHi(room)
    local selfLoX,selfHiX,roomLoX,roomHiX = self:xOverlapValues(room)
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

So we do.

function SegmentPair:overlapLoHi()
    local selfLoX,selfHiX,roomLoX,roomHiX = self.room1:xOverlapValues(self.room2)
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

Max and min fail, because in Room, I had this:

local min = math.min
local max = math.max

Copy that over. Tests all still passing. Now I’d like to have my own version of overlapValues. That would be like this:

function SegmentPair:overlapLoHi()
    local selfLoX,selfHiX,roomLoX,roomHiX = self:overlapValues()
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

Are you getting the drift? At each level, I first refer back to the original method, using my cached rooms, and then I remove the xy sensitivity and just call self, no longer needing to pass the rooms because we already know them. So we move from:

function Room:xOverlapValues(room)
    local selfLoX, selfHiX = self:xValues()
    local roomLoX, roomHiX = room:xValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

To …

function SegmentPair:overlapValues()
    local selfLoX, selfHiX = self.room1:xValues()
    local roomLoX, roomHiX = self.room2:xValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

Tests pass. Now we want to get rid of the calls to xValues.

Here, I think we have to change our steps a bit. Our object knows two rooms, and ultimately will just know their values somehow. But it’s going to have to ask that question of the rooms. We don’t want to condition on type now, however. But we do have to have two sets of values in hand. So let’s try this. (I’m not certain about this, but sure enough to try it.)

You know what? This runs all the tests. Let’s commit: refactoring into SegmentPair. Now if my idea fails, we just rewind to here.

First I do this:

function SegmentPair:overlapValues()
    local selfLoX, selfHiX = self:room1Values()
    local roomLoX, roomHiX = self:room2Values()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

That invents the notion of the two rooms’ worth of values and creates these two methods to call back:

function SegmentPair:room1Values()
    return self.room1:xValues()
end

function SegmentPair:room2Values()
    return self.room2:xValues()
end

Tests still run. Commit: further SegmentPair refactoring.

Now let’s push those two methods further into our object:

function SegmentPair:room1Values()
    return self.r1values
end

function SegmentPair:room2Values()
    return self.r2values
end

I just made the design decision to cache those values. Ah, but no, I can’t just cache and return a pair. I have to say something like this:

function SegmentPair:room1Values()
    return self.r1Lo, self.r1Hi
end

function SegmentPair:room2Values()
    return self.r2Lo, self.r2Hi
end

This of course doesn’t work because those member variables aren’t there. And I’m questioning myself a bit here. If I cache these values, then rooms can’t move. Now I don’t expect them to move, but that doesn’t mean I want to nail their feet to the floor. Let’s keep it dynamic, this way:

function SegmentPair:room1Values()
    return self:values(self.room1)
end

function SegmentPair:room2Values()
    return self:values(self.room2)
end

We just keep pushing the final answer down and down. This is very much like what one does in good Smalltalk. Now for values, we really have to talk to the room:

function SegmentPair:values(aRoom)
    return aRoom:xValues()
end

Guess what, we’re at the bottom of this call. But we have one more x to remove, and we do it this way:

The tests still run. And we can delete all the x-methods from Room that we’ve implemented so far. That’s xOverlapLoHi and xOverlapValues. We remove those … and we find another reference to xOverlapLoHi:

function Room:xOverlapCenter(aRoom)
    local lo,hi = self:xOverlapLoHi(aRoom)
    return (lo + hi)//2
end

We need to do him the same way as our original, namely:

function Room:xOverlapCenter(aRoom)
    local sp = SegmentPair:forX(self,aRoom)
    local lo,hi = sp:overlapLoHi()
    return (lo + hi)//2
end

Now we can delete xOverlapLoHi, and xOverlapValues, and when the tests run, commit: further SegmentPair refactoring.

Now we can do just the same thing to our y methods, except calling SegmentPair:forY, which doesn’t exist yet … until now:

function SegmentPair:forY(room1,room2)
    return SegmentPair(room1,room2,false)
end

And we can do this:

function Room:yOverlapCenter(aRoom)
    local sp = SegmentPair:forY(self,aRoom)
    local lo,hi = sp:overlapLoHi()
    return (lo + hi)//2
end

And expect the tests to run. They do. And we can do this:

function Room:yOverlapLength(aRoom)
    local sp = SegmentPair:forY(self,aRoom)
    local lo,hi = sp:overlapLoHi()
    return max(hi-lo,0)
end

And the tests run. We can delete these:

function Room:yOverlapLoHi(room)
    local selfLoY,selfHiY,roomLoY,roomHiY = self:yOverlapValues(room)
    return max(selfLoY,roomLoY),min(selfHiY,roomHiY)
end

function Room:yOverlapValues(room)
    local selfLoX, selfHiX = self:yValues()
    local roomLoX, roomHiX = room:yValues()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

And the tests run. We can delete this:

local min = math.min

And I think I’ll delete the other and call math.max. It’s not worth the oddity to do this trick. Done. Tests run.

Now there’s not much x vs y stuff left, but there is some. There’s this, and we call back for these values:

function Room:xValues()
    local xyxy = {self:corners()}
    return xyxy[1],xyxy[3]
end

function Room:yValues()
    local xyxy = {self:corners()}
    return xyxy[2],xyxy[4]
end

Let’s do this instead:

function SegmentPair:forX(room1,room2)
    return SegmentPair(room1,room2,1,3)
end

function SegmentPair:forY(room1,room2)
    return SegmentPair(room1,room2,2,4)
end

function SegmentPair:init(room1, room2, sub1,sub2)
    self.room1 = room1
    self.room2 = room2
    self.sub1 = sub1
    self.sub2 = sub2
end

function SegmentPair:values(aRoom)
    local c = { aRoom:corners() }
    return c[self.sub1],c[self.sub2]
end

And the tests run and we can delete these:

function Room:xValues()
    local xyxy = {self:corners()}
    return xyxy[1],xyxy[3]
end

function Room:yValues()
    local xyxy = {self:corners()}
    return xyxy[2],xyxy[4]
end

I think we’re not done yet, but we’re done for today. Commit: pushing overlap logic to SegmentPair.

We’re left with these two pairs in Room:

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

We might be able to reduce those, but even if we don’t, we’ve removed several blocks of duplication, and produced a nice tidy new helper class:

-- SegmentPair
-- RJ 20201110

SegmentPair = class()

local min = math.min
local max = math.max

-- class methods

function SegmentPair:forX(room1,room2)
    return SegmentPair(room1,room2,1,3)
end

function SegmentPair:forY(room1,room2)
    return SegmentPair(room1,room2,2,4)
end

function SegmentPair:init(room1, room2, sub1,sub2)
    self.room1 = room1
    self.room2 = room2
    self.sub1 = sub1
    self.sub2 = sub2
end

-- instance methods

function SegmentPair:overlapLoHi()
    local selfLoX,selfHiX,roomLoX,roomHiX = self:overlapValues()
    return max(selfLoX,roomLoX),min(selfHiX,roomHiX)
end

function SegmentPair:overlapValues()
    local selfLoX, selfHiX = self:room1Values()
    local roomLoX, roomHiX = self:room2Values()
    return selfLoX, selfHiX, roomLoX, roomHiX
end

function SegmentPair:room1Values()
    return self:values(self.room1)
end

function SegmentPair:room2Values()
    return self:values(self.room2)
end

function SegmentPair:values(aRoom)
    local c = { aRoom:corners() }
    return c[self.sub1],c[self.sub2]
end

Oh, before I go, let me rename the variables in those methods, they smack of their origin.

function SegmentPair:overlapValues()
    local r1Lo,r1Hi = self:room1Values()
    local r2Lo,r2Hi = self:room2Values()
    return r1Lo, r1Hi, r2Lo, r2Hi
end
~~

Tests run.

~~~lua
function SegmentPair:overlapLoHi()
    local r1Lo,r1Hi,r2Lo,r2Hi = self:overlapValues()
    return max(r1Lo,r2Lo),min(r1Hi,r2Hi)
end

Tests run. Commit: general names used in SegmentPair.

That’s better. And I should sum up and publish this baby. I’m so happy I’m going back and changing the blurb, which I usually write before I start.

Summing Up

I’m really pleased with how pushing the overlap logic into SegmentPair has gone. There are two aspects of it that I’d like to touch on, one more briefly than the other.

There is a creative aspect to having thought of SegmentPair as a kind of thing. It came from thinking about line segments overlapping. I’m not saying this was a major invention, but I do think picking it as the abstraction helped with the process. I have no useful advice on how to think of a new abstraction.

The other aspect, though, was that long staged refactoring. The fundamental notion was:

  1. Create a new object to do some job.
  2. Push doing that job into the object bit by bit, calling back to old objects until you don’t need them any more.

I just repeated that over and over until done. Each step, calling in and calling back, and then moving the call back one more level down, left the tests running and the code a bit better. (At least had we deleted all the replaced methods as we went. I felt better about leaving the deletes until the end, but that’s because I’m old and not used to good code management tools.)

This example turned out delightfully tidy and simple, at least from my side of the keyboard. It just went step by step, very simply and repeating the same ideas over and over.

This is not a surprising one-off oh that will never happen again kind of thing. It’s the kind of thing that happens when you use this particular way of refactoring from an old way to a new one. Honestly, I probably don’t do it as often as I should.

It feels slow to work that way. It’s much more macho and cool to start whaling away on your new object and then call it and see whether it works. (It usually doesn’t.) And it’s not quite as exciting as just programming by intention, with tests one hopes, and seeing where the code leaves. It’s just a good solid step by step motion from one place to a better one.

And there’s no way I could have done it without those tests. Without them, my mistakes would not have been noticed until there were masses of new code and masses deleted.

This was a very nice example of refactoring with tests. And I didn’t even know it was going to happen.

See you next time!

D1.zip

  1. He likes to understand. That’s not to say that he always does understand. ↩