First we do a Lens. Then an amazingly smooth, very long, step by step refactoring. I’m proud of this one!

Currently, the Knowledge class has an optional offset pair, x and y, that are used to, well, offset what we get when we ask for x,y. We get x+self.x,y+self.y. This is handy for when the robot has moved, or it seemed so when I wrote it. The current “model” that I have in my mind is that the robot wants to think about objects relative to itself. In addition, the scan operation returns objects relative to the robot position (and rotation, which we’re not dealing with yet). The offset is built into the Knowledge object, and it is persistent. Once you set it, it’ll be used until you change it.

The Lens idea is to change Knowledge to only use absolute coordinates, and then to provide an adapter sort of object that does the offsetting.

I was going to do that today only if time permitted, but I’ve decided to do it first, since I think Lens will help me improve this nasty method, which is the main point of the article:

function World:scan(robotName)
    local knowledge = Knowledge()
    local fact, factContent
    local robot = self._robots[robotName]
    if not robot then return knowledge end
    local rx = robot._x
    local ry = robot._y
    --print("robot pos ", rx,ry)
    for x = -5,5 do
        --print("looking at x's", x+rx,ry)
        factContent = self:factAt(x+rx,ry)
        if factContent then
            --fact = Fact(x+rx,ry,factContent):moveBy(-rx,-ry)
            knowledge:addFactAt(factContent,x,0)
        end
    end
    for y = -5,5 do
        --print("looking at y's", rx,y+ry)
        factContent = self:factAt(rx,y+ry)
        if factContent then
            --fact = Fact(rx,y+ry,factContent):moveBy(-rx,-ry)
            knowledge:addFactAt(factContent,0,y)
        end
    end
    return knowledge
end

I’ve left my debug stuff in there, commented out, because this method works, but is not done. It’s too ugly to be allowed to stay in that form. Since the scan method is doing offsetting (see rx and ry) it might benefit from the presence of the Lens, and my thinking might be improved as well. So, Lens:

Lens

Knowledge will be changed to always work in the same coordinates. It doesn’t know what those are, but we know that they’ll go from -width to width, -height to height, centered at 0,0. Knowledge doesn’t care: it just knows that this thing is at this x,y position. We’ll make that so by removing its x and y member variables, and references to them.

Here’s Knowledge:

Knowledge = class()

function Knowledge:init()
    self.x = 0
    self.y = 0
    self.facts = {}
end

function Knowledge:moveBy(x,y)
    self.x = self.x + x
    self.y = self.y + y
end

function Knowledge:addFactAt(content, x,y)
    self:privateaddFactAt(Fact(x,y,content))
end

function Knowledge:privateaddFactAt(aFact)
    table.insert(self.facts, aFact:moveBy(self.x, self.y))
end

function Knowledge:factAt(x,y)
    for i,fact in ipairs(self.facts) do
        if fact.x - self.x == x and fact.y - self.y ==y then return fact.content end
    end
    return nil
end

function Knowledge:factCount()
    return #self.facts
end

We don’t have much use for factCount, if any, and should perhaps remove it. But we are here for bigger game.

So we change Knowledge in these places:

function Knowledge:init()
    self.facts = {}
end

function Knowledge:moveBy(x,y)
end

function Knowledge:factAt(x,y)
    for i,fact in ipairs(self.facts) do
        if fact.x == x and fact.y == y then return fact.content end
    end
    return nil
end

Tests will break. That’s my plan, I plan to use the breaking tests to guide my creation of Lens.

In fact 11 tests break, which is rather more than I expected, but we’ll just see what’s up. I expect the changes to drive out and use the Lens.

Here’s the first error:

1: Robot updates knowledge on move  -- 
Actual: nil, 
Expected: fact

And the test:

        _:test("Robot updates knowledge on move", function()
            local world = World:test1()
            local robot = Robot("Louie",world)
            robot:scan()
            _:expect(robot:factAt(3,0)).is("fact")
            _:expect(robot:factAt(1,3)).is(nil)
            robot:move(1,1)
            _:expect(robot:factAt(2,-1)).is("fact")
            _:expect(robot:factAt(3,0)).is(nil)
            robot:scan()
            _:expect(robot:factAt(0,2), "after second scan").is("not visible on first scan")
        end)

Kind of a long one. Let’s try the ones in Knowledge first, like this one:

2: Knowledge adjusted  -- 
Actual: nil, 
Expected: A

And the test:

        _:test("Knowledge adjusted", function()
            local knowledge = Knowledge()
            knowledge:addFactAt("A",1,1)
            knowledge:addFactAt("B",1,3)
            knowledge:addFactAt("C",5,1)
            knowledge:addFactAt("D",4,4)
            _:expect(knowledge:factCount()).is(4)
            knowledge:moveBy(2,2)
            _:expect(knowledge:factAt(-1,-1)).is("A")
            _:expect(knowledge:factAt(-1,1)).is("B")
            _:expect(knowledge:factAt(3,-1)).is("C")
            _:expect(knowledge:factAt(2,2)).is("D")
        end)

Yes, that should be easier. And yes, Virginia, selecting the broken test to fix first does require some judgment. Here, the change to the new thinking is pretty straightforward, I think. Knowledge doesn’t do moveBy any more. Instead we want it to return a Lens, which acts like Knowledge but is offset. So:

        _:test("Knowledge adjusted", function()
            local knowledge = Knowledge()
            knowledge:addFactAt("A",1,1)
            knowledge:addFactAt("B",1,3)
            knowledge:addFactAt("C",5,1)
            knowledge:addFactAt("D",4,4)
            _:expect(knowledge:factCount()).is(4)
            local lens = knowledge:lens(2,2)
            _:expect(lens:factAt(-1,-1)).is("A")
            _:expect(lens:factAt(-1,1)).is("B")
            _:expect(lens:factAt(3,-1)).is("C")
            _:expect(lens:factAt(2,2)).is("D")
        end)

This will error on lens:

2: Knowledge adjusted -- TestKnowledge:35: attempt to call a nil value (method 'lens')

Code:

function Knowledge:lens(x,y)
    return Lens(self,x,y)
end

This will demand Lens:

2: Knowledge adjusted -- TestKnowledge:119: attempt to call a nil value (global 'Lens')

Create just the class:

Lens = class()

Expect error on factAt:

2: Knowledge adjusted -- TestKnowledge:36: attempt to call a nil value (method 'factAt')

I could mess around here with Fake It Till You Make It and so on, but let’s try being a bit more clever. I feel up to it.

function Lens:init(knowledge, x, y)
    self._knowledge = knowledge
    self._x = x
    self._y = y
end

function Lens:factAt(x,y)
    return self._knowledge:factAt(self._x+x, self._y+y)
end

I expect that test to pass, and if it doesn’t, well, we’ll see what trying to be clever gets you.

It does pass, so there! Next error:

3: Knowledge adjusted twice  -- 
Actual: nil, 
Expected: A

The test is this:

        _:test("Knowledge adjusted twice", function()
            local knowledge = Knowledge()
            knowledge:addFactAt("A",1,1)
            knowledge:moveBy(2,2)
            _:expect(knowledge:factAt(-1,-1)).is("A")
            knowledge:addFactAt("B",1,1)
            knowledge:moveBy(1,1)
            _:expect(knowledge:factAt(0,0)).is("B")
            _:expect(knowledge:factAt(-2,-2)).is("A")
        end)

This one clearly needs a lens there at the first moveBy. But what about the second? I see no reason why a Lens can’t have a moveBy. Let’s define it that way:

        _:test("Knowledge adjusted twice", function()
            local knowledge = Knowledge()
            knowledge:addFactAt("A",1,1)
            local lens = knowledge:lens(2,2)
            _:expect(lens:factAt(-1,-1)).is("A")
            lens:addFactAt("B",1,1)
            lens:moveBy(1,1)
            _:expect(lens:factAt(0,0)).is("B")
            _:expect(lens:factAt(-2,-2)).is("A")
        end)

I expect the error to be on lens not knowing moveBy, but that’s because I didn’t notice the addFactAt call. We need that also:

3: Knowledge adjusted twice -- TestKnowledge:47: attempt to call a nil value (method 'addFactAt')

So:

function Lens:addFactAt(content,x,y)
    return self._knowledge:addFactAt(content, self._x+x, self._y+y)
end

Now can I get an error on moveBy? Yes:

3: Knowledge adjusted twice -- TestKnowledge:48: attempt to call a nil value (method 'moveBy')

Implement. I can see three ways to do this:

  1. Update this lens’s _x and _y and return;
  2. Return a new lens on _knowledge with adjusted x and y;
  3. Return a new lens looking at this one, allowing them to cascade.

The third option would be really cool, but I think people will be moving a lot, so the cascade could get quite long. I think it’s possible that someone would hold on to one Lens and get another one using moveBy, and expect the first one to stay as it was. So option 2 it is, new Lens:

function Lens:moveBy(dx,dy)
    return Lens(self._knowedge, self._x+dx, self._y+dy)
end

I have high hopes that test 3 will now pass. It does not:

3: Knowledge adjusted twice  -- 
Actual: nil, 
Expected: B
3: Knowledge adjusted twice  -- 
Actual: nil, 
Expected: A

Ah. My bug: I adjusted the lens and used the old one, which didn’t move! This can be fixed readily:

        _:test("Knowledge adjusted twice", function()
            local knowledge = Knowledge()
            knowledge:addFactAt("A",1,1)
            local lens = knowledge:lens(2,2)
            _:expect(lens:factAt(-1,-1)).is("A")
            lens:addFactAt("B",1,1)
            local lens2 = lens:moveBy(1,1)
            _:expect(lens2:factAt(0,0)).is("B")
            _:expect(lens2:factAt(-2,-2)).is("A")
        end)

Now will you run? Well, no:

3: Knowledge adjusted twice -- TestKnowledge:102: attempt to index a nil value (field '_knowledge')

This is why we have tests: they know things that I, and my limited IDE, do not know. The bug is a typo here:

function Lens:moveBy(dx,dy)
    return Lens(self._knowedge, self._x+dx, self._y+dy)
end

Fix that. Test runs.

However, we’ve been taught an important lesson, which is that users (me) are inclined to think that moveBy affects the current lens, rather than returning a new one.

Let’s change that method to newLens, and let’s change it in Knowledge as well:

function Knowledge:newLens(x,y)
    return Lens(self,x,y)
end

function Lens:newLens(dx,dy)
    return Lens(self._knowledge, self._x+dx, self._y+dy)
end

I changed the tests to reflect this. I think I got them all but if I didn’t we’ll hear about it.

OK, the only tests failing now are that first one:

1: Robot updates knowledge on move  -- 
Actual: nil, 
Expected: fact
1: Robot updates knowledge on move  -- 
Actual: fact, 
Expected: nil

The test is:

        _:test("Robot updates knowledge on move", function()
            local world = World:test1()
            local robot = Robot("Louie",world)
            robot:scan()
            _:expect(robot:factAt(3,0)).is("fact")
            _:expect(robot:factAt(1,3)).is(nil)
            robot:move(1,1)
            _:expect(robot:factAt(2,-1)).is("fact")
            _:expect(robot:factAt(3,0)).is(nil)
            robot:scan()
            _:expect(robot:factAt(0,2), "after second scan").is("not visible on first scan")
        end)

Change to use a Lens:

function Robot:move(x,y)
    self.knowledge:moveBy(x,y)
    self._x = self._x + x
    self._y = self._y + y
end

Even with the Lens, we need to think for a moment. I think the Robot’s knowledge should always be a lens, just for clarity. Let’s take a more broad look: robot doesn’t work the way I think it does:

function Robot:init(name,aWorld)
    self._world = aWorld
    self._x, self._y = aWorld:launchRobot(name, self)
    self._name = name
end

function Robot:factAt(x,y)
    return self.knowledge:factAt(x,y)
end

function Robot:move(x,y)
    self.knowledge:moveBy(x,y)
    self._x = self._x + x
    self._y = self._y + y
end

function Robot:scan()
    self.knowledge = self._world:scan(self._name)
end

Currently, robot has no permanent knowledge: it replaces its knowledge with a new one when it does a scan. That won’t last: we really want it to update its own copy as a memory of what it has scanned in the past. For now, however, our job is to preserve its behavior in the face of this refactoring to use Lens. So we just have to do this:

function Robot:move(x,y)
    self.knowledge = self.knowledge:newLens(x,y)
    self._x = self._x + x
    self._y = self._y + y
end

I expect this to make the test run. And it does. Note that Lens can be seen as a kind of Knowledge, so the name is somewhat legit. I think we’re going to wind up wanting to change that. But for now we’ll let it be.

Commit: Knowledge now supports a Lens object providing offset view.

Lens done, at least for now. Now that nasty scan method:

Improve Scan

N.B. This long long refactoring turned out even better than I had imagined. It surprised me. I recommend a careful read, perhaps after a quick scan. Or not, I’m not the boss of you.

The method, in case its ugly image has finally left your mind’s eye, is this:

function World:scan(robotName)
    local knowledge = Knowledge()
    local fact, factContent
    local robot = self._robots[robotName]
    if not robot then return knowledge end
    local rx = robot._x
    local ry = robot._y
    --print("robot pos ", rx,ry)
    for x = -5,5 do
        --print("looking at x's", x+rx,ry)
        factContent = self:factAt(x+rx,ry)
        if factContent then
            --fact = Fact(x+rx,ry,factContent):moveBy(-rx,-ry)
            knowledge:addFactAt(factContent,x,0)
        end
    end
    for y = -5,5 do
        --print("looking at y's", rx,y+ry)
        factContent = self:factAt(rx,y+ry)
        if factContent then
            --fact = Fact(rx,y+ry,factContent):moveBy(-rx,-ry)
            knowledge:addFactAt(factContent,0,y)
        end
    end
    return knowledge
end

We can do better. And, as I suspected, the Lens can help us.

I’m not fond of the fact, no pun intended, that I gave World the factAt and addFactAt methods. Maybe I’ll get over that lack of fondness. We’ll see. Anyway, let’s plug in a Lens to help out:

function World:scan(robotName)
    local knowledge = Knowledge()
    local fact, factContent
    local robot = self._robots[robotName]
    if not robot then return knowledge end
    local rx = robot._x
    local ry = robot._y
    local lens = self._knowledge:newLens(rx,ry)
    for x = -5,5 do
        factContent = lens:factAt(x,0)
        if factContent then
            knowledge:addFactAt(factContent,x,0)
        end
    end
    for y = -5,5 do
        factContent = lens:factAt(0,y)
        if factContent then
            knowledge:addFactAt(factContent,0,y)
        end
    end
    return knowledge
end

I expect the tests to continue to work. They do.

Now let’s refactor. Let’s extract everything after the check for robot:

function World:scan(robotName)
    local knowledge = Knowledge()
    local fact, factContent
    local robot = self._robots[robotName]
    if not robot then return knowledge end
    self:accumulateKnowledge(knowledge)
    return knowledge
end

function World:accumulateKnowledge(knowledge)
    local rx = robot._x
    local ry = robot._y
    local lens = self._knowledge:newLens(rx,ry)
    for x = -5,5 do
        factContent = lens:factAt(x,0)
        if factContent then
            knowledge:addFactAt(factContent,x,0)
        end
    end
    for y = -5,5 do
        factContent = lens:factAt(0,y)
        if factContent then
            knowledge:addFactAt(factContent,0,y)
        end
    end
end

Should still work. Test. Doesn’t: forgot to pass in the robot.

function World:scan(robotName)
    local knowledge = Knowledge()
    local fact, factContent
    local robot = self._robots[robotName]
    if not robot then return knowledge end
    self:accumulateKnowledge(knowledge, robot)
    return knowledge
end

function World:accumulateKnowledge(knowledge, robot)
...

Now they pass. A refactoring tool would have found that for me. I have to rely on my tests.

Now this isn’t ideal …

function World:accumulateKnowledge(knowledge, robot)
    local rx = robot._x
    local ry = robot._y
    local lens = self._knowledge:newLens(rx,ry)
    for x = -5,5 do
        factContent = lens:factAt(x,0)
        if factContent then
            knowledge:addFactAt(factContent,x,0)
        end
    end
    for y = -5,5 do
        factContent = lens:factAt(0,y)
        if factContent then
            knowledge:addFactAt(factContent,0,y)
        end
    end
end

Oh, I should mention a thought I had last night. When we scan, our scan goes “outward” from the robot, and should only return the first thing the scan hits. We’ll work toward that change as we go, but we don’t have a test for it.

I think it won’t matter, as whatever we do will be legitimate refactorings, given the tests we have. We’ll see. A wiser person than I might write the additional tests now, but they wouldn’t pass … we’ll stay on this path a bit.

Extract a method from the mess above:

function World:accumulateKnowledge(knowledge, robot)
    local rx = robot._x
    local ry = robot._y
    local lens = self._knowledge:newLens(rx,ry)
    self:scanEastWest(knowledge,lens)
    for y = -5,5 do
        local factContent = lens:factAt(0,y)
        if factContent then
            knowledge:addFactAt(factContent,0,y)
        end
    end
end

function World:scanEastWest(knowledge, lens)
    for x = -5,5 do
        local factContent = lens:factAt(x,0)
        if factContent then
            knowledge:addFactAt(factContent,x,0)
        end
    end
end

I noticed that I had not made the factContent local in the refactoring. Fixed that as well. Tests should still run. Do the next “obvious” extraction:

function World:accumulateKnowledge(knowledge, robot)
    local rx = robot._x
    local ry = robot._y
    local lens = self._knowledge:newLens(rx,ry)
    self:scanEastWest(knowledge,lens)
    self:scanNorthSound(knowledge, lens)
end

function World:scanEastWest(knowledge, lens)
    for x = -5,5 do
        local factContent = lens:factAt(x,0)
        if factContent then
            knowledge:addFactAt(factContent,x,0)
        end
    end
end

function World:scanNorthSouth(knowledge, lens)
    for y = -5,5 do
        local factContent = lens:factAt(0,y)
        if factContent then
            knowledge:addFactAt(factContent,0,y)
        end
    end
end

Tests should run, and would had I typed South above.

function World:accumulateKnowledge(knowledge, robot)
    local rx = robot._x
    local ry = robot._y
    local lens = self._knowledge:newLens(rx,ry)
    self:scanEastWest(knowledge,lens)
    self:scanNorthSouth(knowledge, lens)
end

We’re green. Let’s commit as this is a decent improvement. Commit: refactoring World:scan.

Continuing Refactoring Scan

Continuing, I don’t quite like the initial split:

function World:scan(robotName)
    local knowledge = Knowledge()
    local fact, factContent
    local robot = self._robots[robotName]
    if not robot then return knowledge end
    self:accumulateKnowledge(knowledge, robot)
    return knowledge
end

function World:accumulateKnowledge(knowledge, robot)
    local rx = robot._x
    local ry = robot._y
    local lens = self._knowledge:newLens(rx,ry)
    self:scanEastWest(knowledge,lens)
    self:scanNorthSouth(knowledge, lens)
end

There’s no value to passing in a robot, let’s pass in the Lens from here. And lets rename accumulate to something scanny.

First this:

function World:scan(robotName)
    local robot = self._robots[robotName]
    local newKnowledge = Knowledge()
    if robot then
        local lens = self._knowledge:newLens(robot._x, robot._y)
        self:accumulateKnowledge(newKnowledge, lens)
    end
    return newKnowledge
end

function World:accumulateKnowledge(knowledge, lens)
    self:scanEastWest(knowledge,lens)
    self:scanNorthSouth(knowledge, lens)
end

That’s nicer throughout. Then rename:

function World:scan(robotName)
    local robot = self._robots[robotName]
    local newKnowledge = Knowledge()
    if robot then
        local lens = self._knowledge:newLens(robot._x, robot._y)
        self:scanNSEWinto(newKnowledge, lens)
    end
    return newKnowledge
end

function World:scanNSEWinto(knowledge, lens)
    self:scanEastWest(knowledge,lens)
    self:scanNorthSouth(knowledge, lens)
end

I added “into” to connote that we are accumulating. I think we can do better with the names.

function World:scan(robotName)
    local robot = self._robots[robotName]
    local accumulatedKnowledge = Knowledge()
    if robot then
        local lens = self._knowledge:newLens(robot._x, robot._y)
        self:scanNSEWinto(accumulatedKnowledge, lens)
    end
    return accumulatedKnowledge
end

function World:scanNSEWinto(accumulatedKnowledge, lens)
    self:scanNorthSouth(accumulatedKnowledge, lens)
    self:scanEastWest(accumulatedKnowledge,lens)
end

function World:scanEastWest(accumulatedKnowledge, lens)
    for x = -5,5 do
        local factContent = lens:factAt(x,0)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,x,0)
        end
    end
end

function World:scanNorthSouth(accumulatedKnowledge, lens)
    for y = -5,5 do
        local factContent = lens:factAt(0,y)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,0,y)
        end
    end
end

Interesting thought: as these are written now, they’ll find the current robot, it it were actually in the world, at 0,0. We surely don’t want that. For values of surely. And don’t …

Let’s try changing one of those to go one way and then the other.

function World:scanEastWest(accumulatedKnowledge, lens)
    for x = 1,5 do
        local factContent = lens:factAt(x,0)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,x,0)
        end
    end
    for x = -1,-5,-1 do
        local factContent = lens:factAt(x,0)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,x,0)
        end
    end
end

Tests still green. Now we’re not going to see the robot on the EW scan. Do the same to NS

function World:scanNorthSouth(accumulatedKnowledge, lens)
    for y = 1,5 do
        local factContent = lens:factAt(0,y)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,0,y)
        end
    end
    for y = -1,-5,-1 do
        local factContent = lens:factAt(0,y)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,0,y)
        end
    end
end

We seem to have generated a lot of duplication. We can extract it:

function World:scanEastWest(accumulatedKnowledge, lens)
    for x = 1,5 do
        scanFactInto(accumulatedKnowledge,x,0)
    end
    for x = -1,-5,-1 do
        local factContent = lens:factAt(x,0)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,x,0)
        end
    end
end

function scanFactInto(accumulatedKnowledge,x,y)
    local factContent = lens:factAt(x,y)
    if factContent then
        accumulatedKnowledge:addFactAt(factContent,x,y)
    end
end

New method scanFactInto accepts x and y. Seemed reasonable. Test. Needs lens, tiny fool.

function World:scanEastWest(accumulatedKnowledge, lens)
    for x = 1,5 do
        scanFactInto(accumulatedKnowledge,lens, x,0)
    end
    for x = -1,-5,-1 do
        local factContent = lens:factAt(x,0)
        if factContent then
            accumulatedKnowledge:addFactAt(factContent,x,0)
        end
    end
end

function scanFactInto(accumulatedKnowledge,lens, x,y)
    local factContent = lens:factAt(x,y)
    if factContent then
        accumulatedKnowledge:addFactAt(factContent,x,y)
    end
end

Test. Green. Substitute throughout:

function World:scanEastWest(accumulatedKnowledge, lens)
    for x = 1,5 do
        self:scanFactInto(accumulatedKnowledge,lens, x,0)
    end
    for x = -1,-5,-1 do
        self:scanFactInto(accumulatedKnowledge,lens, x, 0)
    end
end

function World:scanFactInto(accumulatedKnowledge,lens, x,y)
    local factContent = lens:factAt(x,y)
    if factContent then
        accumulatedKnowledge:addFactAt(factContent,x,y)
    end
end

Did you notice that I accidentally made it a naked function? Neither did I. Worked, of course, but wrong … except that we’re seeing methods here that do not use any member variables of the defining class. There’s a class trying to be born here.

Continue refactoring with the other two:

function World:scanNorthSouth(accumulatedKnowledge, lens)
    for y = 1,5 do
        self:scanFactInto(accumulatedKnowledge,lens, 0,y)
    end
    for y = -1,-5,-1 do
        self:scanFactInto(accumulatedKnowledge,lens, 0,y)
    end
end

Test: Green. Need save point. Commit: further World:scan refactoring.

We could have been committing more often. Decisions decisions.

I see duplication here. There’s a general guideline that a method should either make a decison, run a loop, or just call other methods. These NorthSouth EastWest things have two for loops. We can extract those:

function World:scanEastWest(accumulatedKnowledge, lens)
    self:scanEast(accumulatedKnowledge, lens)
    self:scanWest(accumulatedKnowledge, lens)
end

function World:scanEast(accumulatedKnowledge, lens)
    for x = 1,5 do
        self:scanFactInto(accumulatedKnowledge,lens, x,0)
    end
end

function World:scanWest(accumulatedKnowledge, lens)
    for x = -1,-5,-1 do
        self:scanFactInto(accumulatedKnowledge,lens, x, 0)
    end
end

Nicer. But there’s still duplication between East and West (and we haven’t done North South yet). Let’s do those because there is something odd there.

OK, first we have this:

function World:scanNSEWinto(accumulatedKnowledge, lens)
    self:scanNorthSouth(accumulatedKnowledge, lens)
    self:scanEastWest(accumulatedKnowledge,lens)
end

function World:scanEastWest(accumulatedKnowledge, lens)
    self:scanEast(accumulatedKnowledge, lens)
    self:scanWest(accumulatedKnowledge, lens)
end

function World:scanNorthSouth(accumulatedKnowledge, lens)
    self:scanNorth(accumulatedKnowledge, lens)
    self:scanSouth(accumulatedKnowledge, lens)
end

No need for those two-by methods. Let’s do this:

function World:scanNSEWinto(accumulatedKnowledge, lens)
    self:scanNorth(accumulatedKnowledge, lens)
    self:scanSouth(accumulatedKnowledge, lens)
    self:scanEast(accumulatedKnowledge, lens)
    self:scanWest(accumulatedKnowledge, lens)
end

Tests are green. Now I see a lot of duplication here:

function World:scanEast(accumulatedKnowledge, lens)
    for x = 1,5 do
        self:scanFactInto(accumulatedKnowledge,lens, x,0)
    end
end

function World:scanWest(accumulatedKnowledge, lens)
    for x = -1,-5,-1 do
        self:scanFactInto(accumulatedKnowledge,lens, x, 0)
    end
end

function World:scanNorth(accumulatedKnowledge, lens)
    for y = 1,5 do
        self:scanFactInto(accumulatedKnowledge,lens, 0,y)
    end
end

function World:scanSouth(accumulatedKnowledge, lens)
    for y = -1,-5,-1 do
        self:scanFactInto(accumulatedKnowledge,lens, 0,y)
    end
end

Let’s see how far we can push this. Consider just these two:

function World:scanEast(accumulatedKnowledge, lens)
    for x = 1,5 do
        self:scanFactInto(accumulatedKnowledge,lens, x,0)
    end
end

function World:scanWest(accumulatedKnowledge, lens)
    for x = -1,-5,-1 do
        self:scanFactInto(accumulatedKnowledge,lens, x, 0)
    end
end

Those only differ by the for clause parameters. I wonder what we could do about that. Let me begin by shortening that name:

function World:scanEast(accK, lens)
    for x = 1,5 do
        self:scanFactInto(accK,lens, x,0)
    end
end

And then:

function World:scanEast(accK, lens)
    self:scanXFactsInto(accK,lens, {1,5})
end

And then:

function World:scanXFactsInto(accK,lens, r)
    for x = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, x, 0)
    end
end

Nice. Now we can do the other direction as well:

function World:scanWest(accK, lens)
    self:scanXFactsInto(accK,lens, {-1,-5,-1})
end

I expect green, and get it. Let me do NorthSouth similarly and then let’s review.

However, wait. I failed to complete one of those renames to accK and no test failed. We do not test around all sides of our robot. Tests are weak. I think we’re still good, but we do need a more robust test or two.

Let’s do the right thing and write it:

Improve Tests

We’ll test through Robot.

        _:test("Scan on all four sides", function()
            local world = World()
            local robot = Robot("Louie",world)
            world:addFactAt("right",5,0)
            world:addFactAt("left", -4,0)
            world:addFactAt("up", 0,3)
            world:addFactAt("down",0,-2)
            robot:scan()
            _:expect(robot:factAt(5,0)).is("right")
            _:expect(robot:factAt(-4,0)).is("left")
            _:expect(robot:factAt(0,3)).is("up")
            _:expect(robot:factAt(0,-2)).is("down")
        end)

This test runs. Much better. Commit: refactoring World:scan.Improving tests.

Now then, back to the refactoring:

function World:scanNorth(accK, lens)
    self:scanYFactsInto(accK,lens, {1,5,1})
end

function World:scanYFactsInto(accK, lens, r)
    for y = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, 0,y)
    end
end

Test. Green. Repeat:

function World:scanSouth(accK, lens)
    self:scanYFactsInto(accK,lens, {-1,-5,-1})
end

Tests green. Commit: refactor world:scan down to scanX and YFactsInto.

Let’s review what we have. We’re not done yet:

X and Y Seem Similar …

function World:scanEast(accK, lens)
    self:scanXFactsInto(accK,lens, {1,5})
end

function World:scanWest(accK, lens)
    self:scanXFactsInto(accK,lens, {-1,-5,-1})
end

function World:scanNorth(accK, lens)
    self:scanYFactsInto(accK,lens, {1,5,1})
end

function World:scanSouth(accK, lens)
    self:scanYFactsInto(accK,lens, {-1,-5,-1})
end

function World:scanXFactsInto(accK,lens, r)
    for x = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, x, 0)
    end
end

function World:scanYFactsInto(accK, lens, r)
    for y = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, 0,y)
    end
end

I wonder whether we can remove that duplication in those last two methods. Let’s make them more similar by renaming the iteration value to, oh, xy.

function World:scanXFactsInto(accK,lens, r)
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, xy, 0)
    end
end

function World:scanYFactsInto(accK, lens, r)
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, 0, xy)
    end
end

Wow, those are really similar, but not quite. Let’s do something else:

function World:scanXFactsInto(accK,lens, r)
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, 1*xy, 0*xy)
    end
end

function World:scanYFactsInto(accK, lens, r)
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, 0*xy, 1*xy)
    end
end

Now the parameters to scanFactInto are both xy, but then multiplied by 1 or 0 as needed. Tests: Still green.

Move the multipliers up into locals:

function World:scanXFactsInto(accK,lens, r)
    local mx,my = 1,0
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, mx*xy, my*xy)
    end
end

function World:scanYFactsInto(accK, lens, r)
    local mx,my = 0,1
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, mx*xy, my*xy)
    end
end

Tests: green.

My those are similar, aren’t they? Let’s make mx and my parameters and set them in the calls:

function World:scanEast(accK, lens)
    self:scanXFactsInto(accK,lens, {1,5}, 1, 0)
end

function World:scanWest(accK, lens)
    self:scanXFactsInto(accK,lens, {-1,-5,-1}, 1, 0)
end

function World:scanNorth(accK, lens)
    self:scanYFactsInto(accK,lens, {1,5,1}, 0,1)
end

function World:scanSouth(accK, lens)
    self:scanYFactsInto(accK,lens, {-1,-5,-1}, 0,1)
end

function World:scanXFactsInto(accK,lens, r, mx, my)
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, mx*xy, my*xy)
    end
end

function World:scanYFactsInto(accK, lens, r, mx, my)
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, mx*xy, my*xy)
    end
end

Now these are totally identical. We only need one:

function World:scanEast(accK, lens)
    self:scanFactsInto(accK,lens, {1,5}, 1, 0)
end

function World:scanWest(accK, lens)
    self:scanFactsInto(accK,lens, {-1,-5,-1}, 1, 0)
end

function World:scanNorth(accK, lens)
    self:scanFactsInto(accK,lens, {1,5,1}, 0,1)
end

function World:scanSouth(accK, lens)
    self:scanFactsInto(accK,lens, {-1,-5,-1}, 0,1)
end

function World:scanFactsInto(accK,lens, r, mx, my)
    for xy = r[1],r[2],r[3] or 1 do
        self:scanFactInto(accK,lens, mx*xy, my*xy)
    end
end

Tests green. Commit: All World:scan comes down to scanFactsInto.

Are we finished? Well, we could be. But … if we were to pass in a negative mx or my, we could always iterate forward …

Let’s do that:

function World:scanEast(accK, lens)
    self:scanFactsInto(accK,lens, {1,5}, 1,0)
end

function World:scanWest(accK, lens)
    self:scanFactsInto(accK,lens, {1,5}, -1,0)
end

function World:scanNorth(accK, lens)
    self:scanFactsInto(accK,lens, {1,5}, 0,1)
end

function World:scanSouth(accK, lens)
    self:scanFactsInto(accK,lens, {1,5}, 0,-1)
end

Well isn’t that interesting! We don’t need to pass in the loop range any more. We can just do this:

function World:scanEast(accK, lens)
    self:scanFactsInto(accK,lens, 1,0)
end

function World:scanWest(accK, lens)
    self:scanFactsInto(accK,lens, -1,0)
end

function World:scanNorth(accK, lens)
    self:scanFactsInto(accK,lens, 0,1)
end

function World:scanSouth(accK, lens)
    self:scanFactsInto(accK,lens, 0,-1)
end

function World:scanFactsInto(accK,lens, mx, my)
    for xy = 1,5 do
        self:scanFactInto(accK,lens, mx*xy, my*xy)
    end
end

Fascinating. Can we do better? I think we can. Are we tired yet? Not really.

There sure is a lot of duplication there among scanNorth, South, East, and West. We like to remove duplication.

Let’s begin by packaging up the mx and my, like this:

function World:scanEast(accK, lens)
    self:scanFactsInto(accK,lens, {mx=1,my=0})
end

function World:scanWest(accK, lens)
    self:scanFactsInto(accK,lens, {mx=-1,my=0})
end

function World:scanNorth(accK, lens)
    self:scanFactsInto(accK,lens, {mx=0,my=1})
end

function World:scanSouth(accK, lens)
    self:scanFactsInto(accK,lens, {mx=0,my=-1})
end

function World:scanFactsInto(accK,lens, mtab)
    for xy = 1,5 do
        self:scanFactInto(accK,lens, mtab.mx*xy, mtab.my*xy)
    end
end

We just packed mx and my into a little table. Tests still green. Let’s rename that table:

function World:scanFactsInto(accK,lens, direction)
    for xy = 1,5 do
        self:scanFactInto(accK,lens, direction*xy, direction.my*xy)
    end
end

Direction. Makes sense. North is mx=0,my=1. Seems like north to me. Let’s rename scanFactsInto to scanInDirection:

function World:scanEast(accK, lens)
    self:scanInDirection(accK,lens, {mx=1,my=0})
end

function World:scanWest(accK, lens)
    self:scanInDirection(accK,lens, {mx=-1,my=0})
end

function World:scanNorth(accK, lens)
    self:scanInDirection(accK,lens, {mx=0,my=1})
end

function World:scanSouth(accK, lens)
    self:scanInDirection(accK,lens, {mx=0,my=-1})
end

function World:scanInDirection(accK,lens, direction)
    for xy = 1,5 do
        self:scanFactInto(accK,lens, direction.mx*xy, direction.my*xy)
    end
end

Hmm, nice. Now suppose we did this:

“Final” Version

function World:scan(robotName)
    local robot = self._robots[robotName]
    local accumulatedKnowledge = Knowledge()
    if robot then
        local lens = self._knowledge:newLens(robot._x, robot._y)
        self:scanNSEWinto(accumulatedKnowledge, lens)
    end
    return accumulatedKnowledge
end

function World:scanNSEWinto(accK, lens)
    local direction = {
        east= {mx= 1,my= 0},
        west= {mx=-1,my= 0},
        north={mx= 0,my= 1},
        south={mx= 0,my=-1}
    }
    self:scanInDirection(accK,lens, direction.north)
    self:scanInDirection(accK,lens, direction.south)
    self:scanInDirection(accK,lens, direction.east)
    self:scanInDirection(accK,lens, direction.west)
end

function World:scanInDirection(accK,lens, direction)
    for xy = 1,5 do
        self:scanFactInto(accK,lens, direction.mx*xy, direction.my*xy)
    end
end

function World:scanFactInto(accumulatedKnowledge,lens, x,y)
    local factContent = lens:factAt(x,y)
    if factContent then
        accumulatedKnowledge:addFactAt(factContent,x,y)
    end
end

Compare that to the original:

function World:scan(robotName)
    local knowledge = Knowledge()
    local fact, factContent
    local robot = self._robots[robotName]
    if not robot then return knowledge end
    local rx = robot._x
    local ry = robot._y
    for x = -5,5 do
        factContent = self:factAt(x+rx,ry)
        if factContent then
            knowledge:addFactAt(factContent,x,0)
        end
    end
    for y = -5,5 do
        factContent = self:factAt(rx,y+ry)
        if factContent then
            knowledge:addFactAt(factContent,0,y)
        end
    end
    return knowledge
end

The original is one 20 line method. Now we have four methods, totalling 34 lines including blank lines. Each method is easy to understand on its own, and you don’t have to look deeper if you don’t want to, because the names are pretty evocative of what’s going on.

I’d freely grant that the mx,my +/-1 thing is a bit of a trick, but it eliminates two backward loops and lets everything fold down into one method. And if you’ll go back and review how we got there, it was in very tiny steps, each of which was clearly correct. The conversion to use of mx and my went in three or four steps, and while the motivation may not have been clear—it wasn’t entirely clear to me, by the way—it was all aimed at making similar methods more and more alike until suddenly they were equal.

Frankly, this was about as smooth as it gets. I am proud of this one and urge the patient reader to track it carefully. (And if there’s something unclear, please drop me a message and I’ll see if I can do better.)