Robot 5

I have a bit more time and an urge to do some code improvement. Let’s see what we can do to make all this a bit easier.

One of the things I was taught to ask myself is whether the objects I’m working with are helping me. In the case of the Knowledge and Fact objects, I feel that they are not as helpful as they might be. Lel’s muse about why.

First of all, as a use of Knowledge, I have no interest in what kind of object it uses to store things. I do have an interest in the x and y coordinates of things, with a caveat, but the object called Fact seems like none of my business.

As for the coordinates, I’d rather like not to worry about those so much either, although there is a sort of X-and-Y-ness to the whole issue of the world and the Robot’s surroundings.

Let’s think about surroundings. It seems to me that the Robot always wants to think about things relative to itself. It doesn’t care about the absolute coordinates of things, only relative ones. We somewhat accommodate that with the x and y offsets in Knowledge, which are used internally to save and restore things. Let’s review the code:

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:addFact(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

I note that in other objects, I’ve started to use the convention of prefixing member variables with underbar. We might want to be consistent about that. The main reason I do it is to allow me to call methods foo() rather than getFoo. In Knowledge we have no such concern, but we should probably be consistent anyway.

This morning I spoke of a “lens” on the Knowledge, that would manage the offsets. In particular, in the Knowledge of the World, we’d like to shift the focus of the lens to that of the robot we’re currently serving.

I have other concerns on my mind. I don’t feel that the current storage scheme is particularly robust. We could insert two Facts at the same x and y, and we’d only ever find the first one. In the case of World, where we are given array ranges to store obstacles and such, we don’t even check for overlap in any way at all.

This might argue for a more robust scheme inside Knowledge. This causes me to wonder whether equal vectors in Codea are identical as regards indexing in tables. I think not, and this new test proves it:

        _:test("Equal vectors are not identical", function()
            local tab = {}
            local v1 = vec2(3,4)
            local v2 = vec2(3,4)
            _:expect(v1,"not even equal").is(v2)
            tab[v2] = "wrong"
            tab[v1] = "hello"
            _:expect(tab[v2]).is("wrong")
        end)

If v2 and v1 were identical, tab[v2] would become “hello” but it does not. So we can’t just use vectors as keys directly. We could store things by row and column, and that might be better.

Let’s first get rid of the reliance on Fact objects outside Knowledge. Let’s keep the Fact in the names of the methods at least for now, but change them so that factAt just returns the contents, and addFact becomes addFactAt(content,x,y). This will be moderately extensive but should be fairly quick. To assist, I’m going to make Fact a local class in Knowledge, which will make all our references outside it fail to run. That should make changes easy to make.

I’ll try to run the tests and field the errors.

10: place and detect one obstacle -- TestWorld:21: attempt to call a nil value (global 'Fact')

function World:addFact(x,y,thing)
    self._knowledge:addFact(Fact(x,y,thing))
end

The code here should be this:

function World:addFact(x,y,thing)
    self._knowledge:addFactAt(thing,x,y)
end

I’m going to want to change World’s function as well, but not yet. I think this change should drive out the new method:

10: place and detect one obstacle -- TestWorld:21: attempt to call a nil value (method 'addFactAt')

For now, I want to add that method to Knowledge, and to rename/remove the old addFact. But first let me comment:

If I had a refactoring browser or IDE, I might approach this differently, because it would make changes all over the app for me. Codea doesn’t have that ability, so I can’t quite do that and I choose to let the tests guide me. But that doesn’t mean I won’t use Codea’s search. In a moment, I think I’ll do that. But first to make this test 10 work.

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

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

I think this will make 10 work and perhaps others fail. Let’s find out. Yes. Test 10 works. Lots of others now say they can’t find Fact, so let’s Search for those and see what’s up.

There are a raft of them Here’s a pic of Codea’s search window:

I think that’s worth doing in Sublime, since I can paste the code right over and use Sublime’s great powers.

The first part of this is easy, just some multi-cursor editing to remove a bit of the line. The next thing is to change the order of the parameters, which is going to require a regex. Or I could just bite the bullet but that’s no fun.

A bit of pattern matching and I’m down to one test not working:

1: Robot updates knowledge on move -- TestWorld:91: attempt to call a nil value (global 'Fact')

That’s our monster code:

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:addFact(fact)
        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:addFact(fact)
        end
    end
    return knowledge
end

We don’t have moveBy at our disposal any more, so I’ll have to do that bit by hand. The moveby method adds to the coordinates, which means it’ll subtract here. So:

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

That’s actually simpler, isn’t it? Unless you ask yourself just why it is the way that it is …

Tests are green. Commit: Make Fact local to Knowledge. replace addFact with addFactAt(content,x,y).

Now let’s replace that other addFact method:

function World:addFact(x,y,thing)
    self._knowledge:addFactAt(thing,x,y)
end

I can leave that and add a new method:

function World:addFactAt(thing, x,y)
    self._knowledge:addFactAt(thing,x,y)
end

Then I can incrementally find all the remaining calls to addFact and replace them. I can do them one at a time and the tests will still run.

function World:test1()
    local world = World()
    world:addFactAt("fact",3,0)
    world:addFact(1,3,"not visible on first scan")
    return world
end

Tests are green. Again:

function World:test1()
    local world = World()
    world:addFactAt("fact",3,0)
    world:addFactAt("not visible on first scan", 1,3)
    return world
end

Tests are green.

        _:test("place and detect one obstacle", function()
            local world = World(25,25)
            world:addFactAt("O", -2,10)
            _:expect(world:factAt(-2,10)).is("O")
        end)

Tests are green. Remove World:addFact entirely. Tests find the one my eyes missed:

function World:addThings(left, top, right, bottom, thing)
    for x = left,right do
        for y = bottom,top do
            self:addFactAt(thing,x,y)
        end
    end
end

Tests are green. Commit: World converted to provide and use addFactAt rather than addFact.

The world (and knowledge) are a slightly better place. The updates were pretty rote and could have been done manually. Using Sublimes regex was easier and faster.

And note, please, the trick of leaving the old addFact method in until all the users have been removed one at a time. Wasn’t necessary here, but in a larger application, we might leave a deprecated method in the system for quite a while. It’s better not to, of course, but sometimes needs must.

So. A couple of nice little refactorings, and soon it will be time for Penne Marinara with Tenderloin bits.

See you next time!