I wouldn’t call them ideas, exactly, but I have some things I want to try.

It’s a seasonal Fall morning, about 0715, I am nice and crisply freshened up, and ready to go. As I said I might, I did draw a few pictures of boxes, and thought about adjacency, but nothing conclusive came out. There is great regularity in the checks we do, and I think they could lend themselves to being expressed in a tabular fashion, but that would surely be more obscure than the code is now. So unless it led to some new convenient “adjacency abstraction”, I don’t think I’d do it, and I don’t see that abstraction at the moment.

I did make some observations about the code, and I want to try to exploit them. Here’s the set of functions I have in mind:

function Room:closeEnoughOnWestTo(aRoom)
    local ce = Room:closeEnough()
    local hisE = aRoom:east()
    local hisW = aRoom:west()
    local myE = self:east()
    local myW = self:west()
    return (hisW>=myE and hisW-myE<ce)
end

function Room:closeEnoughOnEastTo(aRoom)
    local ce = Room:closeEnough()
    local hisE = aRoom:east()
    local hisW = aRoom:west()
    local myE = self:east()
    local myW = self:west()
    return (myW>=hisE and myW-hisE<ce)
end

function Room:closeEnoughOnNorthTo(aRoom)
    local ce = Room:closeEnough()
    local hisN = aRoom:north()
    local hisS = aRoom:south()
    local myN = self:north()
    local myS = self:south()
    return (myS>=hisN and myS-hisN<ce)
end

function Room:closeEnoughOnSouthTo(aRoom)
    local ce = Room:closeEnough()
    local hisN = aRoom:north()
    local hisS = aRoom:south()
    local myN = self:north()
    local myS = self:south()
    return (hisS>=myN and hisS-myN<ce)
end

These look mysteriously similar, but in fact each one is different. There’s duplication here, but it’s odd, a sort of duplication of form without duplicating the values. That does suggest that one could extract some kind of function, at least the one that does the bits in the return statement.

I noticed, however, that these functions–which I absolutely cut and pasted and worked hard to make them similar, so that I had a prayer of seeing if they were correct–these functions all create two locals that they do not use. So the first thing I want to do is to remove the unreferenced locals:

function Room:closeEnoughOnWestTo(aRoom)
    local ce = Room:closeEnough()
    --local hisE = aRoom:east()
    local hisW = aRoom:west()
    local myE = self:east()
    --local myW = self:west()
    return (hisW>=myE and hisW-myE<ce)
end

I’m commenting them out for now, partly to show you what’s changing, but mostly because I’m scared this won’t work, despite the fact that it must work. Having changed that one, I’ll run the tests. And they pass. This gives me confidence to do all the functions with full delete. After all, I’m just a revert away. Commenting out is for chickens.

I did do them one at a time. I’m not a chicken, but I’m not a cliff diver either. So now I have this:

function Room:closeEnoughOnWestTo(aRoom)
    local ce = Room:closeEnough()
    local hisW = aRoom:west()
    local myE = self:east()
    return (hisW>=myE and hisW-myE<ce)
end

function Room:closeEnoughOnEastTo(aRoom)
    local ce = Room:closeEnough()
    local hisE = aRoom:east()
    local myW = self:west()
    return (myW>=hisE and myW-hisE<ce)
end

function Room:closeEnoughOnNorthTo(aRoom)
    local ce = Room:closeEnough()
    local hisN = aRoom:north()
    local myS = self:south()
    return (myS>=hisN and myS-hisN<ce)
end

function Room:closeEnoughOnSouthTo(aRoom)
    local ce = Room:closeEnough()
    local hisS = aRoom:south()
    local myN = self:north()
    return (hisS>=myN and hisS-myN<ce)
end

So those are smaller and that’s good. They are also, of course, grandly similar. But not the same.

But can’t we extract a function anyway? Let’s do one that does the comparison, and that takes responsibility for getting the closeEnough value:

function Room:closeEnoughOnWestTo(aRoom)
    local hisW = aRoom:west()
    local myE = self:east()
    return self:wallsCloseEnough(hisW,myE)
end

function Room:wallsCloseEnough(hisW,myE)
    local ce = Room:closeEnough()
    return hisW>=myE and hisW-myE<ce
end

This works and runs the tests. Plugging it into the others will be delicate, as we have to get the parms in the right order. But with care, it’ll be OK: One at a time.

function Room:closeEnoughOnEastTo(aRoom)
    local hisE = aRoom:east()
    local myW = self:west()
    return self:wallsCloseEnough(myW,hisE)
end

So far so good …

function Room:closeEnoughOnNorthTo(aRoom)
    local hisN = aRoom:north()
    local myS = self:south()
    return self:wallsCloseEnough(myS,hisN)
end

One more to go …

function Room:closeEnoughOnSouthTo(aRoom)
    local hisS = aRoom:south()
    local myN = self:north()
    return self:wallsCloseEnough(hisS,myN)
end

And everything is still working. We are slowly whittling this code down to its essence, and, for me at least, not losing clarity. But we have lots of duplication left here. Each of these functions is fetching a wall coordinate and then passing them in to the wallsCloseEnough function. Can we get rid of those fetches?

I think we can. Let’s try it with one of them.

function Room:closeEnoughOnWestTo(aRoom)
    return self:compareByName(aRoom, self.east,self.west)
end

function Room:compareByName(him, myFetch,hisFetch)
    local ce = Room:closeEnough()
    local mine = myFetch(self)
    local his = hisFetch(him)
    return his>=mine and his-mine<ce
end

This, I admit, is a bit deeper in the bag of tricks. The compareByName takes the other room (not self), and the method to call on self, and the method to call on the other room, to get the values for the compare. Then it calls those methods and does the compare.

We can’t write self:myFetch() because that looks for the method whose name is myFetch, which of course has no associated method.

Let’s rename some variables for what I hope will be improved clarity:

function Room:compareByName(otherRoom, myWall,hisWall)
    local ce = Room:closeEnough()
    local mine = myWall(self)
    local his = hisWall(otherRoom)
    return his>=mine and his-mine<ce
end

I think that’s a bit more clear. Now let’s see how many tries it takes me to change the other functions.

Ah. It only takes me one try to see why this won’t work. And I do wish I had done a commit. Anyway let me command Z back and we’ll talk.

OK, back to where I was. Commit: refactoring closeEnoughOnXXXX functions.

My little trick didn’t work, because of something that isn’t obvious at first. We’re not just changing which wall we fetch when we do these compares, we change the order in which we do the compare. Look closely at these:

function Room:closeEnoughOnWestTo(aRoom)
    local hisW = aRoom:west()
    local myE = self:east()
    return self:wallsCloseEnough(hisW,myE)
end

function Room:closeEnoughOnEastTo(aRoom)
    local hisE = aRoom:east()
    local myW = self:west()
    return self:wallsCloseEnough(myW,hisE)
end

We fetch the opposite walls … and we pass them to the compare in the opposite order. That’s because if you are to the west of me, my west is bigger than and close to your east, but if you are to the east of me, your west is bigger than and close to my east.

So we fetch the opposite walls … and reverse the omparison.

So that fetch trick doesn’t work unless I also somehow reversed the compare, and that’s no fun.

Anyway, it is fortunately 0811, and a good time to break for a chai run. See you in a bit, refreshed.

It’s 0845. Back from chai run. And the break was worthwhile. I had expected to come back and say “oh well, not every idea is a good one”. Instead, I’m back thinking this idea can actually work. Let’s find out.

First, I’ll put back what I had:

function Room:closeEnoughOnWestTo(aRoom)
    return self:compareByName(aRoom, self.east,self.west)
end

function Room:compareByName(him, myFetch,hisFetch)
    local ce = Room:closeEnough()
    local mine = myFetch(self)
    local his = hisFetch(him)
    return his>=mine and his-mine<ce
end

This much works. The trick will be to make it work for the East version. It starts like this:

function Room:closeEnoughOnEastTo(aRoom)
    local hisE = aRoom:east()
    local myW = self:west()
    return self:wallsCloseEnough(myW,hisE)
end

Recall that the problem is that not only do we fetch different walls, but we have to compare them in the opposite order.

Let’s rename the parameters in the new method a bit. I’m going to try to get them meaningful enough that I can think clearly about what I’m doing.

I guess I’ll stick with the “wall” names but I think we can do a bit more:

function Room:compareByName(otherRoom, selfWallLo, otherWallHi)
    local ce = Room:closeEnough()
    local low = selfWallLo(self)
    local high = otherWallHi(otherRoom)
    return high>=low and high-low<ce
end

I am hoping this will keep clear in my mind the two key aspects here, whose wall it is, and whether we expect it to be high or low in coordinate value.

Now can I make east work?

The trick I have in mind is not to call self:compare(aRoom) but aRoom:compare(self). I think if I do that and get the fetches right, it’ll work. We’ll debate later whether it’s good.

It does work. Here are the West and East to compare:

function Room:closeEnoughOnWestTo(aRoom)
    return self:compareByName(aRoom, self.east,self.west)
end

function Room:closeEnoughOnEastTo(aRoom)
    return aRoom:compareByName(self,self.east,self.west)
end

Note that I reversed the call, and also the two directions, to ensure that we fetch the correct value from the correct object. This is a bit obscure, but for now I’m going with it. Here go the other two, one at a time.

function Room:closeEnoughOnSouthTo(aRoom)
    return self:compareByName(aRoom, self.north, self.south)
end

This one was easier because it’s not the reversed one. But now:

function Room:closeEnoughOnNorthTo(aRoom)
    return aRoom:compareByName(self, self.north, self.south)
end

Better commit: convert to compareByName.

Now I think the self.north stuff is a bit confusing, as it draws the eye over to “who is self and why do we never say aRoom”. Let’s do this to hopefully make it more clear:

function Room:compareByName(otherRoom, selfWallLo, otherWallHi)
    local ce = Room:closeEnough()
    local low = selfWallLo(self)
    local high = otherWallHi(otherRoom)
    return high>=low and high-low<ce
end

function Room:closeEnoughOnWestTo(aRoom)
    return self:compareByName(aRoom, Room.east, Room.west)
end

function Room:closeEnoughOnEastTo(aRoom)
    return aRoom:compareByName(self, Room.east, Room.west)
end

function Room:closeEnoughOnNorthTo(aRoom)
    return aRoom:compareByName(self, Room.north, Room.south)
end

function Room:closeEnoughOnSouthTo(aRoom)
    return self:compareByName(aRoom, Room.north, Room.south)
end

I had wanted to define, locally, N, S, E, W as Room.north and so on, but those values can only be set up after the Room has compiled. We could do ti in a class init or some such thing. I’m tempted to rename those methods N, S, E, and W but that seems somewhere between unnecessary and worse. We’ll let this ride. Commit: completed conversion to compareByName.

Now let’s reflect a bit.

Reflection

We started this morning with … 32 lines in four groups, containing much duplication. We finish with 18 lines and far less duplication. And the code has moved closer to my ideal, tiny methods doing very little.

That said, while the meaning of what we’re doing is pretty clear, with things like self:closeEnoughOnEastTo(aRoom), how it actually works is a bit mysterious. That’s true for two reasons.

First, we’ve passed two functions into our root compareByName method, and we rarely pass functions around. So that is uncommon and involves some unusual but perfectly legitimate syntax.

Second, the way we call the method is itself a bit mysterious, because sometimes we call self:compare(aRoom) and sometimes aRoom:compare(self).

The result is five short methods that should really be marked “leave these the hell alone”, because they are intimately connected and just confusing enough to make you think you can improve them by swapping a few things around. But you can’t.

So this code is more compact, and it has centralized more of the weirdness, and removed some other weirdness. Look at the four return statements we used to have:

    return (hisW>=myE and hisW-myE<ce)
    return (myW>=hisE and myW-hisE<ce)
    return (myS>=hisN and myS-hisN<ce)
    return (hisS>=myN and hisS-myN<ce)

That’s not exactly the crowning achievement of clarity either, and it was spread around. Now at least the weirdness is more together.

My coding standards, and notions on what is clear code and what isn’t, say that this code can be allowed to stand. I could imagine an individual or team making a different decision. Either way, I think an individual or team that went through the process of thinking hard about the adjacency issue, and trying various ways of representing it, would benefit from the experience.

And they’d be glad it was over. I am glad it’s over too.

If it is over. There’s more code in here not to like. But I’m feeling the need to move more toward something resembling a game.

We’ll do that next time. For now, have fun and see you next time!

D1.zip