Robot 14
Let’s review some code.
Yesterday, I felt pretty good about the work we did on look/scan, which has resulted in the robot keeping a memory of everything it has seen, and displaying it on screen. Compare that feeling to Friday, where I had it working but didn’t feel that the code was suitable, and reverted it back out.
Seriously, I offer that we do well to be sensitive to “good enough”, and when things aren’t good enough, we should revert them out rather than commit them to the repo. When we’re working in small enough steps, we can even resist the notion of saving the code on a branch and trying to work it into shape tomorrow. I suggest that doing it over is often a better way to get it right.
Something to think about.
Anyway, today, I plan to scan all the code and give it a bit of a review. But first, something has happened that I didn’t tell you about.
Matrix
You’ll recall that in Robot 10, I created a little 2x2 rotation matrix, as part of improving the RotationLens. Well, I got to thinking that with the robot moving and turning and such, that it would probably be better to have a full on 2D transformation matrix, which is a 3x3 matrix combining rotation and translation. Codea does have a 3D transformation matrix, and it would be perfectly fine to use it in 2D, but a reader might not have such a thing available. So, one afternoon, I coded it up.
Let’s have a look there first. I’ll paste it in in its entirety, for the record.
-- TestMatrices
-- RJ 20220616
function test_Matrices()
_:describe("Matrices", function()
CodeaUnit_Detailed = false
_:before(function()
end)
_:after(function()
end)
_:test("Multiply matrix times vector", function()
local m = Matrix({ {1,2,3}, {4,5,6}, {7,8,9}})
local v = vec2(1,10)
local w = m*v
_:expect(w.x).is(24)
_:expect(w.y).is(60)
end)
_:test("Translate Vector", function()
local tr = Matrix{ {1,0,2}, {0,1,3}, {0,0,1} }
local v = vec2(3,5)
local w = tr*v
_:expect(w.x).is(5)
_:expect(w.y).is(8)
end)
_:test("multiply two matrices", function()
local m1 = Matrix{ {1,2}, {3,4}}
_:expect(Matrix:isMatrix(m1),"m1 matrix").is(true)
local m2 = Matrix{ {3,5}, {7,11} }
_:expect(Matrix:isMatrix(m2),"m2 matrix").is(true)
local m3 = m1*m2
local m = m3.m
a,b,c,d = m[1][1], m[1][2], m[2][1], m[2][2]
_:expect(a).is(17)
_:expect(b).is(27)
_:expect(c).is(37)
_:expect(d).is(59)
end)
_:test("Rotate vectors", function()
local v = vec2(2,3)
local tr = Matrix:rotateRight()
local w = tr*v
_:expect(w.x).is(-3)
_:expect(w.y).is(2)
local tl = Matrix:rotateLeft()
w = tl*v
_:expect(w.x).is(3)
_:expect(w.y).is(-2)
end)
_:test("Translate", function()
local v = vec2(2,3)
local m = Matrix:translate(1,2)
local w = m*v
_:expect(w.x).is(3)
_:expect(w.y).is(5)
end)
end)
end
Matrix = class()
function Matrix:rotateLeft()
return Matrix{ {0,1,0},{-1,0,0}, {0,0,1}}
end
function Matrix:rotateRight()
return Matrix{ {0,-1,0}, {1,0,0}, {0,0,1}}
end
function Matrix:translate(x,y)
return Matrix{ {1,0,x}, {0,1,y}, {0,0,1} }
end
function Matrix:unit()
return Matrix{ {1,0,0}, {0,1,0}, {0,0,1}}
end
function Matrix:init(rowArray)
self.m = rowArray
end
function Matrix:__tostring()
local res = "Matrix: "
local rows = self.m
res = res.." "..#rows.." rows\n"
return res
end
function Matrix:__mul(mOrV)
local m1 = self.m
if self:isMatrix(mOrV) then
local m2 = mOrV.m
local res = {}
for i = 1,#m1 do
res[i] = {}
for j = 1,#m2[1] do
res[i][j] = 0
for k = 1,#m2 do
res[i][j] = res[i][j] + m1[i][k]*m2[k][j]
end
end
end
return Matrix(res)
else -- must be vector
--self:require3x3()
local vmatrix = Matrix{ {mOrV.x}, {mOrV.y}, {1} }
local w = self*vmatrix
local ww = w.m
return vec2(ww[1][1],ww[2][1])
end
end
function Matrix:isMatrix(m)
if type(m) ~= "table" then return false end
if m.is_a and m:is_a(Matrix) then return true end
return false
end
You’ll note that I’ve gone to a row-wise representation for the matrix, 3 rows of 3 columns, though the multiply code doesn’t care much. It also doesn’t check for conformability, which really would need to be done in a general case.
As for the __mul code, don’t imagine that I wrote that. I found it somewhere on the Internet, I forget where, some page that has every language known to humanity. I wrote tests for it, typed it in very carefully and ultimately made it work.
It basically plugged right in to the RotatingLens and pretty clearly works.
However.
It seems a bit much, for at least two reasons:
First, this robot program is intended primarily as a learning project for programming students, and I suspect that rotation-translation matrices is outside the learning objectives of the course. It’s a bit esoteric unless you are working in graphics, in which case it’s only a tiny bit of what you need to know.
Second, our robot can only turn 90 degrees, so it can only look directly North, East, South, or West, and it can only move in integer numbers of steps, so it seems that something much simpler could be used to create the Lens. The matrix works, but it’s a bit like putting a V-8 into your lawnmower1.
On the other hand, even turning only 90 degrees means that our coordinates change meaning. What was ahead of us is now on our left, and so on. As we saw in some ad-hoc code that I wrote, x and y swap with each rotation, and signs are changed as well. The ad-hoc code that I wrote to do the job was strange. I’d like to have something simpler, but something we can understand.
I wonder if the people trying this exercise will find neat solutions to tracking motion, and what they’ll be. I suspect that at least some teams will have trouble with the tracking issue if they try to keep track of history.
Anyway, that’s the Matrix. let’s move on to other matters.
Knowledge
The World object needs to record all the obstacles, pits, mines, whatevers that are in the world. It needs to keep track of things by their x and y coordinates in the world, which is a flat square of points. Currently, we have a very weak implementation of that, called Knowledge, which stores a small object called a Fact. A Fact has x,y, and content. The Knowledge doesn’t care what content is, and in fact it could be anything. Currently knowledge is stored in a simple array which is searched when we want to find out what, if anything, is at a given coordinate.
Here are the tests and code.
-- TestStarter
-- RJ 20220608
function test_Knowledge()
_:describe("Knowledge", function()
CodeaUnit_Detailed = false
_:before(function()
end)
_:after(function()
end)
_:test("Knowledge retained", 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)
_:expect(knowledge:factAt(1,1)).is("A")
_:expect(knowledge:factAt(1,3)).is("B")
_:expect(knowledge:factAt(5,1)).is("C")
_:expect(knowledge:factAt(4,4)).is("D")
end)
_: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:newLens(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)
_:test("Knowledge adjusted twice", function()
local knowledge = Knowledge()
knowledge:addFactAt("A",1,1)
local lens = knowledge:newLens(2,2)
_:expect(lens:factAt(-1,-1)).is("A")
lens:addFactAt("B",1,1)
local lens2 = lens:newLens(1,1)
_:expect(lens2:factAt(0,0)).is("B")
_:expect(lens2:factAt(-2,-2)).is("A")
end)
_:test("Thursday understanding", function()
local knowledge = Knowledge()
knowledge:addFactAt("A",-1,0)
knowledge:addFactAt("B", 0,1)
local lens = knowledge:newLens(1,1)
_:expect(lens:factAt(-1, 0)).is("B")
_:expect(lens:factAt(-2,-1)).is("A")
end)
_:ignore("Knowledge can have two things at same place", function()
local knowledge = Knowledge()
knowledge:addFactAt("A",1,1)
knowledge:addFactAt("B",1,1)
_:expect(knowledge:factAt(1,1)).is("A")
_:expect(knowledge:factAt(1,1)).is("B")
end)
end)
end
local Fact = class()
function Fact:init(x,y,content)
self.x = x
self.y = y
self.content = content
end
function Fact:__tostring()
return "Fact("..self.content.."("..tostring(self.x)..","..tostring(self.y).."))"
end
function Fact:moveBy(x,y)
return Fact(self.x+x, self.y+y, self.content)
end
Knowledge = class()
function Knowledge:init()
self.facts = {}
end
function Knowledge:moveBy(x,y)
end
function Knowledge:addFactAt(content, x,y)
self:privateaddFactAt(Fact(x,y,content))
end
function Knowledge:privateaddFactAt(aFact)
table.insert(self.facts, aFact)
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
function Knowledge:factCount()
return #self.facts
end
function Knowledge:newLens(x,y)
return Lens(self,x,y)
end
You’ll notice that there is a moveBy method in Knowledge that does nothing, and a moveBy in fact that adjusts its coordinates. Are these used? Let’s find out, we’re here to review the code.
The answer is no. They must be left over from some earlier time. Remove both. Tests all run. Commit: Remove unused moveBy methods in Fact and Knowledge.
Important tips regarding code review: first, do it with a live code browser so that you can search out answers. Second, when you see something readily fixable, fix it right then. Third, have enough tests so that you can do that.
There’s not much else to see here. If we look at the factAt method, we see that it will return the content of the first fact in the table. If there were two facts with the same coordinates, we’d never find the second one. And when we add something, we don’t check to see if there is duplication. And … we don’t know what we’d do about it if there was duplication.
But we should note that this tells us that Knowledge is a work in progress and may need to be changed as we learn more about the game. It’s small and has tests, so we should be OK.
There’s a reference to Lens in Knowledge, and in fact its definition is really in that tab. Let’s talk about lenses.
Lenses
I got the idea of “Lens” when thinking about the game display, which would show a region around the Robot. Somehow I envisioned us floating up above the world, above the robot, with a lens focused on it, showing everything around it.
That picture would appear the same whether the robot was at coordinates 0,0 or 123,456. Everything would be relative to whatever position we looked at.
The world has things at various coordinates. Suppose that our robot is at (10,10) and the world has thing t1 at (9,10) and t2 at (10,12). We know that the look function is supposed to show things referring to N, S, E, W and a distance. So, if we are looking at true North, a look should return something like (W,1,t1) and (N,2,t2).
Now if we subtract our coordinates from the coordinates of t1 and t2, we get (-1,0) and (0,2). And it’s nearly easy to see how that could turn into W 1 and N 2.
So the Lens records the coordinates of a robot and holds on to a Knowledge. When you ask for the facts next to the robot you ask for x going from -5 to 5 … and the Lens adds the robot coordinates to x and causes you to look in the Knowledge from 5 to 15, which are just exactly the points you need to check:
5,6,7,8,9, 10, 11,12,13,14,15
Lens is very simple: it looks like this:
Lens = class()
function Lens:init(knowledge, x, y)
self._knowledge = knowledge
self._x = x
self._y = y
end
function Lens:addFactAt(content,x,y)
return self._knowledge:addFactAt(content, self._x+x, self._y+y)
end
function Lens:factAt(x,y)
return self._knowledge:factAt(self._x+x, self._y+y)
end
function Lens:factCount()
return self._knowledge:factCount()
end
function Lens:newLens(dx,dy)
return Lens(self._knowledge, self._x+dx, self._y+dy)
end
In essence, all it does is add in the x and y that it holds, to any requests for knowledge, and passes them on.
We do have test for Lens, as you’ll see in the tests already shown for Knowledge.
Then there is RotatingLens.
RotatingLens
When I implemented Lens, I only dealt with the x and y coordinates, not with turns. That was fairly intentional and basically I did it because I didn’t yet know what to do with turns, and I was pretty sure it was complicated. So, once Lens was done, at some point I decided to deal with rotation.
I did to an ad-hoc version of rotation. It turns out to be kind of interesting.
Suppose we are at 0,0, facing north, and there is something above and to our right, at, say, 2,3.
If we turn right 90 degrees, the thing will be at -3,2. Turn again and it will be at -2,-3. Turn again and it is at -2,3. Once more and it is at 2,3 again.
Looking at the pattern, we see that a right turn swaps x and y and negates y:
x,y becomes -y,x
That pattern repeats. So if we call that operation swap, we swap once for every 90 degree turn to the right. A similar pattern occurs for turns to the left. 2,3 becomes 3,-2 and so on.
So, however we do it, we need a lens that can deal with turning. I called it RotatingLens.
My first implementation, if I recall, just remembered the sum of one’s turns, counting right as +1 and left as +3, and did that many swap operations. I replaced that with the matrix code. The Rotating Lens looks like this, with tests:
-- TestRotatingLens
-- RJ 20220608
function test_RotatingLens()
_:describe("Rotating Lens", function()
CodeaUnit_Detailed = false
_:before(function()
end)
_:after(function()
end)
_:test("Right turn moves dead ahead to left", function()
local k = Knowledge()
k:addFactAt("DA",0,1) -- dead ahead
local lens = RotatingLens(k)
_:expect(lens:factAt(0,1),"0 degrees").is("DA")
lens:turn("right")
xa,ya = lens:adjust(-1,0)
_:expect(xa,"xa").is(0)
_:expect(ya,"ya").is(1)
_:expect(lens:factAt(-1,0), "90").is("DA") -- off to left
end)
_:test("Left turn moves dead ahead to right", function()
local k = Knowledge()
k:addFactAt("DA",0,1) -- dead ahead
local lens = RotatingLens(k)
lens:turn("left")
_:expect(lens:factAt(1,0),"270").is("DA")
end)
_:test("Two left turns move dead ahead to read", function()
local k = Knowledge()
k:addFactAt("DA",0,5) -- dead ahead
local lens = RotatingLens(k)
lens:turn("left")
lens:turn("left")
_:expect(lens:factAt(0,-5),"180").is("DA")
end)
_:test("Off-axis points", function()
local k = Knowledge()
k:addFactAt("1,2",1,2)
k:addFactAt("-2,3",-2,3)
k:addFactAt("-3,-4",-3,-4)
k:addFactAt("4,-5", 4,-5)
local lens = RotatingLens(k)
lens:turn("right")
_:expect(lens:factAt(-2,1)).is("1,2")
_:expect(lens:factAt(5,4)).is("4,-5")
_:expect(lens:factAt(-3,-2)).is("-2,3")
_:expect(lens:factAt(4,-3)).is("-3,-4")
end)
end)
end
RotatingLens = class()
function RotatingLens:init(knowledge)
self.knowledge = knowledge
self.matrix = Matrix:unit()
end
function RotatingLens:adjust(x,y)
local v = self.matrix*vec2(x,y)
return v.x,v.y
end
function RotatingLens:factAt(x,y)
xa,ya = self:adjust(x,y)
return self.knowledge:factAt(xa,ya)
end
function RotatingLens:newLens(x,y)
return self.knowledge:newLens(x,y)
end
function RotatingLens:turn(direction)
-- when robot turns right, world rotates left and vice versa
if direction=="right" then
self.matrix = Matrix:rotateLeft()*self.matrix
elseif direction=="left" then
self.matrix = Matrix:rotateRight()*self.matrix
end
end
All the rigmarole aside, it all comes down to starting with a unit matrix (which does not rotate at all) and whenever we turn right, multiplying by a left rotation, and whenever we turn left, multiplying by a right rotation, and when we are given an input x and y, we create a vector of that, and multiply it by the current matrix and Voila! out comes the rotated coordinates.
Why is this a better way to do it? I’m not convinced that it is, for you, but for me, we’re one step away from combining the Lens and the RotatingLens, because I happen to know that when we take a step x,y in some direction, we can just multiply the x and y into the matrix (at the right place) and carry on.
Wheels turning…
As I think about this, here in our code review, I am not entirely pleased with this matrix idea. It will work, absolutely. I’m sure that if we apply our transformations (moves and turns) as matrices, the resulting matrix will transform coordinates relative to the robot into world coordinates, and that’s just what we want.
It’s just that it’s not transparent. Once we’ve moved and turned a lot, I have no idea what the matrix will look like. I have very litte intuitive confidence that we’ll get what we want. I can’t visualize how it works.
Honestly, the more I look at it and think about it, the less sure I am about it. That’s not a good way to feel. Too much mystery.
Something needs to be done about this. Maybe Matrix has to be replaced with something else. The Lenses, I like. The way they are made … I don’t like.
Let’s move on.
Robot
-- TestRobot
-- RJ 20220608
function test_Robot()
_:describe("Robot", function()
CodeaUnit_Detailed = false
_:before(function()
end)
_:after(function()
end)
_: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)
_: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)
_:test("Set Up Game", function()
local robot = Robot:setUpGame()
_:expect(robot:factAt(-5,0)).is(nil)
_:expect(robot:factAt(3,0)).is(nil)
robot:scan()
_:expect(robot:factAt(-5,0)).is("OBSTACLE")
_:expect(robot:factAt(3,0)).is("PIT")
end)
_:test("Robot does scan on s key", function()
local world = World()
world:addFactAt("O", 3,0)
local robot = Robot("Louie", world)
_:expect(robot:factAt(3,0)).is(nil)
robot:keyboard("s")
_:expect(robot:factAt(3,0)).is("O")
end)
_:test("Robot moves forward on 1 key", function()
local world = World()
world:addFactAt("O",3,1)
local robot = Robot("Louie", world)
_:expect(robot._y).is(0)
robot:scan()
_:expect(robot:factAt(3,0)).is(nil)
robot:keyboard("1")
_:expect(robot._y).is(1)
robot:scan()
_:expect(robot:factAt(3,0)).is("O")
end)
_:test("Robot understands new 'look'", function()
local world = World()
local robot = Robot("Louie", world)
local look = LookPacket("N", 3, "OBSTACLE")
robot:addLook(look)
_:expect(robot:factAt(0,3)).is("OBSTACLE")
end)
_:test("Robot ignores bad packet direction", function()
local world = World()
local robot = Robot("Louie", world)
robot:move(0,1)
local look = LookPacket("Q", 3, "OBSTACLE")
robot:addLook(look)
_:expect(robot.knowledge:factCount()).is(0)
end)
end)
end
Robot = class()
function Robot:setUpGame()
local world = World:setUpGame()
return Robot("Louie", world)
end
function Robot:init(name,aWorld)
self._world = aWorld
self._x, self._y = aWorld:launchRobot(name, self)
self._name = name
self.knowledge = Knowledge()
end
function Robot:addLook(lookPacket)
local steps = lookPacket:distance()
local item = lookPacket:type()
local dir = lookPacket:direction()
local x,y = self:convertToXY(dir,steps)
self:addFactAt(item,x,y)
end
function Robot:convertToXY(dir,steps)
local convert = { N={0,1}, S={0,-1}, E={1,0}, W={-1,0} }
local mul = convert[dir]
if not mul then return nil,nil end
return steps*mul[1], steps*mul[2]
end
function Robot:addFactAt(item,x,y)
if not x or not y then return end
self.knowledge:addFactAt(item,x,y)
end
function Robot:factAt(x,y)
return self.knowledge:factAt(x,y)
end
function Robot:keyboard(key)
if key == "s" then
self:scan()
elseif key == "1" then
self:move(0,1)
elseif key == "r" then
local lens = RotatingLens(self.knowledge)
lens:turn("right")
self.knowledge = lens
end
end
function Robot:move(x,y)
self.knowledge = self.knowledge:newLens(x,y)
self._x = self._x + x
self._y = self._y + y
end
function Robot:scan()
local packets = self._world:scan(self._name)
for i,packet in ipairs(packets) do
self:addLook(packet)
end
end
Robot has lots of tests. I like the tests. And the Robot code is rather simple and pretty clear.
However, addLook and convertToXY do not seem to belong on Robot. convertToXY has no references to any member variables of Robot, and refers only to members of the LookPacket that is being torn apart in addLook. And addLook just messes around with the packet into until it has it torn apart far enough to get x, y and item, so that it can do an addFactAt.
We only recently discovered LookPacket, and I think here we are discovering that LookPacket should be helping us with all this. There’s no real reason why Robot should know anything about the LookPacket: it’s just still acting as if it was getting that dictionary that it had to tear apart.
As we review this code, we see that we need to move function from Robot to LookPacket. This is “feature envy”, Robot doing something that LookPacket should do for it.
Shall we fix that? We shall.
Refactoring between Robot and LookPacket
Let’s arrange things so that we can say this:
function Robot:addLook(lookPacket)
local item,x,y = lookPacket:decode()
self:addFactAt(item,x,y)
end
We might want to change that, but it’s pretty direct. So …
Hm, whatever I did, didn’t work. Revert, do again, smaller steps.
We have this:
function Robot:addLook(lookPacket)
local steps = lookPacket:distance()
local item = lookPacket:type()
local dir = lookPacket:direction()
local x,y = self:convertToXY(dir,steps)
self:addFactAt(item,x,y)
end
function Robot:convertToXY(dir,steps)
local convert = { N={0,1}, S={0,-1}, E={1,0}, W={-1,0} }
local mul = convert[dir]
if not mul then return nil,nil end
return steps*mul[1], steps*mul[2]
end
Let’s move the convert over to LookPacket first this time:
function LookPacket:convertToXY(dir,steps)
local convert = { N={0,1}, S={0,-1}, E={1,0}, W={-1,0} }
local mul = convert[dir]
if not mul then return nil,nil end
return steps*mul[1], steps*mul[2]
end
function Robot:addLook(lookPacket)
local steps = lookPacket:distance()
local item = lookPacket:type()
local dir = lookPacket:direction()
local x,y = lookPacket:convertToXY(dir,steps)
self:addFactAt(item,x,y)
end
This ought to work. It does. Simplify: LookPacket has the info, no need to pass it:
function Robot:addLook(lookPacket)
local item = lookPacket:type()
local x,y = lookPacket:convertToXY()
self:addFactAt(item,x,y)
end
And then …
function LookPacket:convertToXY()
local convert = { N={0,1}, S={0,-1}, E={1,0}, W={-1,0} }
local mul = convert[self._direction]
if not mul then return nil,nil end
return self._distance*mul[1], self._distance*mul[2]
end
That works. We could commit. However …
function Robot:addLook(lookPacket)
local item,x,y = lookPacket:convertToXY()
self:addFactAt(item,x,y)
end
And …
function LookPacket:convertToXY()
local convert = { N={0,1}, S={0,-1}, E={1,0}, W={-1,0} }
local mul = convert[self._direction]
if not mul then return nil,nil end
return self._type, self._distance*mul[1], self._distance*mul[2]
end
Test. Good. Rename method:
function Robot:addLook(lookPacket)
local item,x,y = lookPacket:asFactDetails()
self:addFactAt(item,x,y)
end
function LookPacket:asFactDetails()
local convert = { N={0,1}, S={0,-1}, E={1,0}, W={-1,0} }
local mul = convert[self._direction]
if not mul then return nil,nil end
return self._type, self._distance*mul[1], self._distance*mul[2]
end
Green. Nice. Commit: refactor Robot:addLook to get decent support from LookPacket.
Super. Again, if we are reviewing the code live, we can make necessary changes as we go. Yes, this makes the code review go longer, but in a mob programming format, it would be more entertaining than pure review, would gain the benefit of all the minds in the room, and would get the actual work done sooner and better.
More person hours per line? Maybe. Who cares? I don’t. Most of the time was spent learning and discussing anyway.
Now let’s look at World.
World
First the tests. Scan these, let’s get to the code.
World Tests
function test_World()
_:describe("World", function()
CodeaUnit_Detailed = false
_:before(function()
end)
_:after(function()
end)
_: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)
_:test("Create Default World", function()
local world = World()
end)
_:test("3x5 world width is 3", function()
local world = World(3,5)
_:expect(world:width()).is(3)
end)
_:test("5x7 world width is 5", function()
local world = World(5,7)
_:expect(world:width()).is(5)
end)
_:test("6x8 world width is 7", function()
local world = World(6,8)
_:expect(world:width()).is(7)
end)
_:test("6x8 world height is 9", function()
local world = World(6,8)
_:expect(world:height()).is(9)
end)
_:test("Lua mod odd behavior", function()
local mod = 5.5%2
_:expect(mod).is(1.5)
end)
_:test("fractions are lost", function()
local world = World(5+1/3, 6.5)
_:expect(world:width()).is(5)
_:expect(world:height()).is(7)
end)
_:test("negatives are zero", function()
local world = World(-51.43, -37.29)
_:expect(world:width()).is(1)
_:expect(world:height()).is(1)
end)
_: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)
_:test("rectangular obstacle", function()
local world = World(25,25) -- -12 to 12, thanks guys
world:createObstacle(-2,10, 3,8)
for x = -2,3 do
for y = 8,10 do
_:expect(world:factAt(x,y)).is("OBSTACLE")
end
end
end)
_:test("erroneous obstacle calls", function()
local world = World(5,5) -- -2 to 2 thanks guys
world:createObstacle(1,1,2,2)
checkWorldEmpty(world, "not top left")
world = World(5,5)
world:createObstacle(-5,6, 5,-6)
checkWorldEmpty(world, "outside limits")
end)
_:test("rectangular pit", function()
local world = World(25,25) -- -12 to 12, thanks guys
world:createPit(-2,10, 3,8)
for x = -2,3 do
for y = 8,10 do
_:expect(world:factAt(x,y)).is("PIT")
end
end
end)
_:test("World can create LookPacket from scan", function()
local world = World(25,25)
world:createPit(0,3,0,3) -- N 3
world:createPit(0,-4,0,-4) -- S 4
world:createObstacle(2,0,2,0) -- E 2
world:createObstacle(-5,0,-5,0) -- W 5
local lens = world._knowledge:newLens(0,0)
local packet = world:createPacketFor(lens, 0,3)
_:expect(packet:direction()).is("N")
_:expect(packet:distance()).is(3)
_:expect(packet:type()).is("PIT")
local packet = world:createPacketFor(lens, 0,-4)
_:expect(packet:direction()).is("S")
_:expect(packet:distance()).is(4)
_:expect(packet:type()).is("PIT")
local packet = world:createPacketFor(lens, 2,0)
_:expect(packet:direction()).is("E")
_:expect(packet:distance()).is(2)
_:expect(packet:type()).is("OBSTACLE")
local packet = world:createPacketFor(lens, -5,0)
_:expect(packet:direction()).is("W")
_:expect(packet:distance()).is(5)
_:expect(packet:type()).is("OBSTACLE")
end)
end)
end
-- test support function
function checkWorldEmpty(world,message)
for x = world:left(),world:right() do
for y = world:bottom(),world:top() do
_:expect(world:factAt(x,y),message).is(nil)
end
end
end
World
Here’s the actual code
-- RJ 20220608
World = class()
function World:test1()
local world = World()
world:addFactAt("fact",3,0)
world:addFactAt("not visible on first scan", 1,3)
return world
end
function World:setUpGame()
local world = World(20,20)
-- left top right bottom
world:createObstacle(-5,3,-5,-3)
world:createPit(3,2,3,-2)
return world
end
function World:init(width, height)
self._width = self:validDimension(width or 1)
self._height = self:validDimension(height or 1)
self._knowledge = Knowledge()
self._robots = {}
end
function World:addFactAt(thing, x,y)
self._knowledge:addFactAt(thing,x,y)
end
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
function World:bottom()
return -(self._height-1)/2
end
function World:createObstacle(left, top, right, bottom)
self:createThings(left, top, right, bottom, "OBSTACLE")
end
function World:createPacketFor(lens,x,y)
local content = lens:factAt(x,y)
if not content then return nil end
local dir = self:directionTo(x,y)
local steps = math.max(math.abs(x), math.abs(y))
return LookPacket(dir, steps, content)
end
function World:directionTo(x,y)
if x > 0 then return "E" end
if x < 0 then return "W" end
if y > 0 then return "N" end
return "S"
end
function World:createPit(left, top, right, bottom)
self:createThings(left, top, right, bottom, "PIT")
end
function World:createThings(left, top, right, bottom, thing)
if self:isValidRectangle(left,top,right,bottom) then
self:addThings(left, top, right, bottom, thing)
end
end
function World:factAt(x,y)
return self._knowledge:factAt(x,y)
end
function World:height()
return self._height
end
function World:isValidRectangle(left,top,right,bottom)
local function inOrder(a,b,c,d)
return a<=b and b<=c and c<=d
end
return inOrder(self:left(),left,right,self:right()) and
inOrder(self:bottom(),bottom, top, self:top())
end
function World:launchRobot(name, robot)
self._robots[name]=robot
return 0,0
end
function World:left()
return -(self._width-1)/2
end
function World:right()
return (self._width-1)/2
end
function World:scan(robotName)
local robot = self._robots[robotName]
local accumulatedKnowledge = {}
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
function World:scanFactInto(accK, lens, x,y)
table.insert(accK, self:createPacketFor(lens,x,y))
end
function World:top()
return (self._height-1)/2
end
function World:validDimension(aNumber)
local nonNegativeInteger = math.max(0,aNumber)//1
local bumpUpEvenValues = nonNegativeInteger + (1 - nonNegativeInteger%2)
return bumpUpEvenValues
end
function World:width()
return self._width
end
The first thing I notice is that my usual alphabetic order for methods doesn’t serve me quite so well here. It’s not terrible, but I think I’d like top bottom left right width height to be together and separated from the more interesting methods. Unfortunately, if we’re to do that, they should probably go right after init, but I think it’s worth doing.
Discussion? No objections. Here goes:
-- domensions
function World:bottom()
return -(self._height-1)/2
end
function World:height()
return self._height
end
function World:left()
return -(self._width-1)/2
end
function World:right()
return (self._width-1)/2
end
function World:top()
return (self._height-1)/2
end
function World:width()
return self._width
end
-- methods
Commit: gather dimensions methods together
OK, I think that will reduce clutter and keep me from wondering where the others are. Now what’s interesting in the code?
I notice we have createObstacle or Pit, goes to createThings, goes to addThings, goes to addFactAt.
The group wonders why those have two kinds of names and suggests that they should all be named “add” something. Let’s do that.
We’ll need to be cautious, as there are callers to these.
There is an alternative: we could build the new methods with the “add” names, and call them from the old “create” ones. Then we could remove the old callers at our convenience. In a larger system, we might do that, deprecating the old methods and changing them to send users emails saying “Per Our Previous Email, ‘create’ is deprecated.” Or something like that. Here, I think we can just do it.
We already have an addThings but you can see that createThings should probably be named addValidThings since it ignores invalid things.
We get this:
--- adding things to the world
function World:addFactAt(thing, x,y)
self._knowledge:addFactAt(thing,x,y)
end
function World:addObstacle(left, top, right, bottom)
self:addValidThings(left, top, right, bottom, "OBSTACLE")
end
function World:addPit(left, top, right, bottom)
self:addValidThings(left, top, right, bottom, "PIT")
end
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
function World:addValidThings(left, top, right, bottom, thing)
if self:isValidRectangle(left,top,right,bottom) then
self:addThings(left, top, right, bottom, thing)
end
end
--
Test. We’re good. Commit: rename thing creation methods to add, group them.
What else do we see?
This seems odd:
function World:createPacketFor(lens,x,y)
local content = lens:factAt(x,y)
if not content then return nil end
local dir = self:directionTo(x,y)
local steps = math.max(math.abs(x), math.abs(y))
return LookPacket(dir, steps, content)
end
function World:directionTo(x,y)
if x > 0 then return "E" end
if x < 0 then return "W" end
if y > 0 then return "N" end
return "S"
end
Here again we see feature envy: these methods have almost nothing to do with World. However, there is an issue that we can’t see but that comes to my mind. We are not yet really handling turning, and if we turn, then the directionTo function will be wrong, and the turn information will surely be in the Lens, one way or another.
This is work in progress, surely going to change. I suggest that we comment it, because our rule is that a comment is the code’s way of asking to be improved, so when we see the comments, we’ll know to try to do the right thing. Perhaps by then, we’ll know more. Surely we won’t know less.
-- createPacketFor and directionTo
-- are in progress and need to support turns
-- seems like feature envy of some kind
Commit: mark troubled code in World with comments.
What else?
isValidRectangle could perhaps be moved up with the add methods. It may be OK here in the regular alpha list. Reviewers do not agree, no one is willing to go to the mats over it.
launchRobot clearly needs work, but we’re not really there yet.
The scan code catches our eye:
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
function World:scanFactInto(accK, lens, x,y)
table.insert(accK, self:createPacketFor(lens,x,y))
end
There are two implementations of scanFactInto. We recognize, and the programmer recalls, that the first one was left for reference, and possible reversion, while building the nice new clean second one. Remove the first one. Commit: remove redundant method.
Now we look at the scan code itself …
Someone points out that accumulatedKnowledge was perhaps a good name in the past but now what we have is an array of LookPacket instances. We could rename like this:
function World:scan(robotName)
local robot = self._robots[robotName]
local packets = {}
if robot then
local lens = self._knowledge:newLens(robot._x, robot._y)
self:scanNSEWinto(packets, lens)
end
return packets
end
function World:scanNSEWinto(packets, 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(packets,lens, direction.north)
self:scanInDirection(packets,lens, direction.south)
self:scanInDirection(packets,lens, direction.east)
self:scanInDirection(packets,lens, direction.west)
end
function World:scanInDirection(packets,lens, direction)
for xy = 1,5 do
self:scanFactInto(packets,lens, direction.mx*xy, direction.my*xy)
end
end
function World:scanFactInto(packets, lens, x,y)
table.insert(packets, self:createPacketFor(lens,x,y))
end
Righteous. We commit: rename accumulatedKnowledge to packets.
That’s about all there is to World except for this validDimension, which ensures that we always have an odd number of rows and columns in the world. Part of the spec.
I think our code review is done. Let’s sum up.
Summary
I’d argue that a team programming, ensemble, mob programming session like we did here would be a good way to look over the code, notice issues, and deal with them, since unlike a classical code review, where you read the code in advance, note down a couple of small points unless you hate the person whose code is being reviewed, in which case you write down some really vicious points, and then you wait, bored out of your skull, until that code is on the screen to make your points, and then you make your points, and then the programmer makes a note of your points, or writes obscenities, no one can read their handwriting anyway, and finally the meeting is over and everyone returns to their desk or goes home, except for the ones who go out drinking, and perhaps someone improves the code later but probably not2 …
What we do here is review the code live, and improve it live. The meeting is more interesting and it’s actually productive in that better code comes out than went in.
Fun, the code’s better, and it’s time for breakfast.
See you next time!
-
If you’re reading this in a few years, a V-8 was a large motor that burned dinosaurs, destroyed the planet, and was much too large for a lawnmower. In those days, there were plants that grew around our houses, and rather than let them get tall, we had machines called lawnmowers that trimmed them down short. Machines were, well, it’s hard to explain, but those big rust heaps you scavenge around used to actually be able to do useful things. ↩
-
Cool, huh? ↩