Dungeon 22

Technical debt, or messy code? Why not both?

In the preceding article, I commented at length on the fact that the code I have here isn’t helping me as much as it could be. Major bullet points included:

• I still don’t turn soon enough to TDD.
• The Room object is too big.
• There are inconsistencies in how things are done.
• The jostling really should be done outside the draw logic.

And I’m sure there are more. Often I believe that I am composed largely of flaws, arranged into a seemingly somewhat competent package.

What we have here at present is a combination of at least two ingredients.

True Technical Debt

When Ward Cunningham coined the phrase “technical debt”, he did not have in mind crufty code. He had in mind a specific condition, which is that after we’ve worked with a program for a while, and kept its design as neatly as we’re capable of, there often come points in time where we have design insights and understanding in our heads that are not in the code. Here’s an example:

At one point, the dungeon game had rooms, and things I called Doors, which are really just representations of places where connections between rooms could be made. Then I was given an insight by Andrew, that I could create new rooms which would serve as hallways between the rooms. That was a useful insight, and after several attempts, it’s now working.

But I realized yesterday that the Doors aren’t quite right. Either they should also be a kind of room, perhaps requiring a key to go through, or they should just be a supporting object deep in the system, perhaps a RoomConnection or something.

I don’t have the answer yet, but my design understanding is beginning to see a different arrangement of things than the one we have. The one we have isn’t wrong or grubby in any conventional sense: this just looks to be a better design than the one we have.

That’s technical debt.

Messy Code

The Room object itself, on the other hand, is over four times larger than any other object in the system, and includes several specialized and mostly independent notions, including jostling, path-finding, floor-tiling, determining neighbors, checking for intersections with other Rooms, and more.

This is too much stuff, too much different stuff, to belong all in one object.

The result is that Room, which is surely still quite central in upcoming stories, is difficult to work with. It’s hard to find the code that needs changing, it contains all kinds of built-in decisions that need to be remembered, and it’s just too bloody long.

That’s not technical debt. It’s just poorly crafted code, resulting in inferior design. Oh, it came on quite reasonably, one little bit at a time. But it’s not good and it needs to be better.

What should we do about this?

This is always the question. Part of me wants to say that I’m nearly done messing with the Room. One more little change, and I’m sure the princess will be able to walk between rooms. We fix that, and we back away slowly. Sure, Room isn’t perfect but it’s working and we can move on.

But there’s a lot more that needs to be done with Room. It needs to contain monsters and treasures and puzzles and farmer boys for the princess to rescue and maybe even a pony. Almost everything that happens in the game from now on will happen in a Room. And Ron’s thinking we won’t have to edit it much, and anyway it’s not that bad, and if we clean it up it’ll just slow us down, and mayb everything will be OK?

Yeah, I’d like to get some money down on the Don’t on that one.

A Tentative Plan

Let’s at least take a look at the design, Room in particular, and see whether we can come up with some quick improvements. Along the way, we’ll make a point of at least glancing at the tests to see whether we need some additional ones.

And I think I’d like to change Main so that the whole dungeon is created, jostled, and linked before we even draw. But what would be something simple to start with? I always like to start with something simple.

I almost think I can do the adjustment in (almost) one go.

We have this:

function draw()
    if DisplayExpanded then
        focusOnRoomOne()
    end
    drawDungeonCreation()
end

function drawDungeonCreation()
    background(40, 40, 50)
    if CodeaUnit then showCodeaUnitTests() end
    pushMatrix()
    pushStyle()
    for i,r in ipairs(Rooms) do
        r:draw()
    end
    Avatar:draw()
    adjustPositions()
    popStyle()
    popMatrix()
end

function adjustPositions()
    if AdjustCount > 0 then
        if ElapsedTime - AdjustTime > AdjustDelay then
            AdjustTime = ElapsedTime
            AdjustCount = Room:adjustAll(Rooms)
            if AdjustCount == 0 then
                Room:setAllToInteger(Rooms)
                Room:createFloors(Rooms)
                Rooms[1]:colorNeighborsIn(Rooms, color(255,0,0,100))
                createRoomsFromDoors()
            end
        end
    end
end

What would happen if we were to move the call to adjust positions up to setup and turn the loop into a while? Let’s try.

function adjustPositions()
    AdjustCount = 1
    while AdjustCount > 0 do
        AdjustCount = Room:adjustAll(Rooms)
    end
    Room:setAllToInteger(Rooms)
    Room:createFloors(Rooms)
    Rooms[1]:colorNeighborsIn(Rooms, color(255,0,0,100))
    createRoomsFromDoors()
end

And we call it here:

function setup()
    if CodeaUnit then
        codeaTestsVisible(true)
        runCodeaUnitTests()
    end
    Rooms = Room:createRooms(50)
    AllDoors = {}
    AdjustTime = ElapsedTime
    AdjustDelay = 0.00
    AdjustCount = 1
    DisplayExpanded = false
    adjustPositions()
    Avatar = Player(1,0,0)
    showKeyboard()
    local seed = math.random(1234567)
    --seed = 1028003
    math.randomseed(seed)
    print(seed)
end

And it works a treat. I wonder why I had trouble with that earlier. Anyway, commit: adjustRooms done before display.

Now let’s see if we can find something to improve in Room. I mentioned use of Rectangle in yesterday’s article. It seems that using the Rectangle object to determine whether room rectangles intersect might be useful.

Let’s have a look at that. That starts in Room:adjustAllRooms, and it goes like this:

function Room:adjustAll(rooms)
    count = 0
    for i,room in ipairs(rooms) do
        if room:moveAwayFrom(rooms) then
            count = count + 1
        end
    end
    return count
end

function Room:moveAwayFrom(rooms)
    local v = self:combinedImpactOf(rooms)
    if v == vec2(0,0) then return false end
    local pos = self:center() + v
    self.x = pos.x
    self.y = pos.y
    return true
end

function Room:combinedImpactOf(allRooms)
    local rooms = self:intersectors(allRooms)
    if #rooms == 0 then return vec2(0,0) end
    local imp = vec2(0,0)
    for i,r in ipairs(rooms) do
        imp = imp + self:impactOf(r)
    end
    return imp/#rooms
end

function Room:impactOf(aRoom)
    local d = self:dist(aRoom)
    if d == 0 then return vec2(0,0) end
    return (self:center()-aRoom:center()):normalize()
    -- used to divide by dist to adjust impact
end

function Room:intersectors(rooms)
    local int = {}
    for i,r in ipairs(rooms) do
        if self ~= r and self:intersects(r) then
            table.insert(int,r)
        end
    end
    return int
end

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

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

Well, this isn’t much, but we should probably move the intersects to Rectangle. Do we have tests for this?

We have some, which gives me confidence we can do this:

function Room:intersects(aRoom)
    return self:rectangle():intersects(aRoom:rectangle())
end

We have this method already:

function Room:rectangle()
    return Rectangle(self.x,self.y, self.w,self.h)
end

Of course this will fail, because rectangles don’t know whether they intersect … yet.

Room:340: attempt to call a nil value (method 'intersects')
stack traceback:
	Room:340: in method 'intersects'
	Room:346: in method 'intersectors'
	Room:209: in method 'combinedImpactOf'
	Room:381: in method 'moveAwayFrom'
	Room:11: in method 'adjustAll'
	Main:61: in function 'adjustPositions'
	Main:15: in function 'setup'

Super, just as expected. Now how about starting from that intersect corners code?

function Rectangle:intersects(aRectangle)
    x1lo, y1lo, x1hi, y1hi = self:corners()
    x2lo, y2lo, x2hi, y2hi = aRectangle:corners()
    if y1lo > y2hi or y2lo > y1hi then return false end
    if x1lo > x2hi or x2lo > x1hi then return false end
    return true
end

With this in place, the tests all run again and the map looks good.

This is a righteous change, in that it locates more decisions about rectangles in the Rectangle object, it reduces the code size of Room, which is overly bulky. There’s not much if any net saving in lines of code, but finding things among Rectangle’s four methods is easier than among Room’s … 53. Wow.

Commit: use Rectangle:intersects for Room intersection.

I’m reminded that I made a discovery that I wanted to talk about.

Pretty-Printing

In a few of the preceding articles, I’ve started to give some of the complex objects, like Room and Rectangle and Player, a better print method. Since Codea just prints a table ID for objects, error messages from the tests are often less than helpful.

It turns out that when Codea/Lua goes to print something, it will call __tostring on the object. If the object understands the message, Codea prints the string that it returns. If not, it uses its default print.

I installed those prints like this:

function Door:init(room1, room2, isPrimary)
    self.isPrimary = isPrimary
    self.room1 = room1
    self.room2 = room2
    if room1.number > room2.number then
        self.room2,self.room1 = room1,room2
    end
    self.room1:addDoor(self)
    self.room2:addDoor(self)
    self.mt = getmetatable(self)
    self.mt.__tostring = self.toString
end

function Door:toString()
    return string.format("%d->%d %s", self.room1.number, self.room2.number, self:yesNo())
end

What that does is fetch the object’s “metatable”, saves it in a member variable mt, saves an element in the metabable named __tostring, pointing to the function toString. I basically copied an example from the internet to do this. But this is redundant and repetitive and extra work. All we really need to do is this:

function Door:__tostring()
    return string.format("%d->%d %s", self.room1.number, self.room2.number, self:yesNo())
end

Right. that puts the name __tostring into the object’s metatable: that’s how objects work. So I can delete that .mt junk. Much less weird-looking, much less for readers to wonder about.

The same trick works for equality checking. Codea calls __eq before it does its own more dumb are these two tables identical check. So with those changes in the system, I’ve got somewhat tidier code and more valuable tests.

Commit: removed metatablle access.

So that’s nice.

Moving Right Along

Still cruising Room looking for things to improve. There’s this:

function RoomPosition:clamp()
    local lox,loy,hix,hiy = self.room:localCorners()
    local x = clamp(lox,self.x,hix)
    local y = clamp(loy,self.y,hiy)
    return RoomPosition(self.room,x,y)
end

function clamp(lo,val,hi)
    return math.max(math.min(val,hi), lo)
end

What we’re doing here is forcing the room position to be inside some bounds, namely the bounds of the room. We can certainly do it here, but arguably this should be a function on rectangle. Or possibly a function on a class we don’t even have, Point.

We have this in Rectangle:

function Rectangle:contains(x,y)
    local result = x <= self.x + self.w/2 and y <= self.y + self.h/2
    and self.x - self.w/2 <= x and self.y - self.h/2 <= y
    return result
end

Rather often in this program, I’ve broken out the x and y of a point, or the four corners of a rectangle, or the x y and width and height of a rectangle, into separate variables. That’s probably not a clever thing to do. It leads to “primitive obsession”, the tendency to write lots of code in terms of primitives, in this case numbers, where objects might serve us better.

Moving away from this could get rather tricky, but might be worth doing if we can go in small steps. I am inclined to posit an object Point, in spite of the fact that we have the vec2 object which serves rather like one. It’s entirely possible to add methods to vec2 if we need to, but that’s rather deep in the bag of tricks. If we’re going to move in the direction or binding our x’s and y’s together, we should build a new class for it. The vec2 class is just another primitive to obsess over.

I’ll do TDD on Point when I need it, but I feel OK creating it for now:

~~~lua– Point – RJ 20201130

Point = class()

function Point:init(x,y) self.x = x self.y = y end

function Point:__tostring() return string.format(“Point (%.2f,%.2f)”, self.x, self.y) end

function Point:__eq(aPoint) return self.x == aPoint.x and self.y == aPoint.y end

Now to use it. I'd like to have Rectangle have a method `containsPoint`, as such:

~~~lua
function Rectangle:containsXY(x,y)
    local result = x <= self.x + self.w/2 and y <= self.y + self.h/2
    and self.x - self.w/2 <= x and self.y - self.h/2 <= y
    return result
end

function Rectangle:containsPoint(aPoint)
    return self:containsXY(aPoint.x, aPoint.y)
end

I’ve renamed the original Rectangle:contains to containsXY, for what I hope will be clarity. Now to find references to contains and fix them:

function Room:acceptRoomPosition(aRoomPosition)
    local rect = self:rectangle()
    local x,y = aRoomPosition:screenLocation()
    if rect:contains(x,y) then
        local newRP = RoomPosition:fromScreenLocation(self, x,y)
        return true, newRP
    else
        return false,aRoomPosition
    end
end

This could be better this way:

function Room:acceptRoomPosition(aRoomPosition)
    if self:rectangle():containsRoomPosition(aRoomPosition) then
        local newRP = RoomPosition:fromScreenLocation(self, x,y)
        return true, newRP
    else
        return false,aRoomPosition
    end
end

But I’m not sure we want our low-level class Rectangle knowing how to convert things like RoomPositions.

For now, let’s get a point from RoomPosition:

function Room:acceptRoomPosition(aRoomPosition)
    local rect = self:rectangle()
    local pt = aRoomPosition:asScreenPoint()
    if rect:containsPoint(pt) then
        local newRP = RoomPosition:fromScreenLocation(self, x,y)
        return true, newRP
    else
        return false,aRoomPosition
    end
end

We now need asScreenPoint. And I really think this should be done with either vectors or points.

function RoomPosition:asScreenPoint()
    return self.room:centerScreenPoint() + self:asLocalPoint()
end

function RoomPosition:asLocalPoint()
    return Point(self.x,self.y)
end

function Room:centerScreenPoint()
    return Point(self.x,self.y)
end

Now this will explode because points do not understand how to add.

function Point:__add(aPoint)
    return Point(self.x+aPoint.x,self.y+aPoint.y)
end

This is a bit deep in the bag of tricks but I’m thinking it’ll be OK.

I do have a bunch of tests failing for calling contains, which now lo longer exists.

Most of those require a change to containsXY, but there’s also this:

function Room:acceptRoomPosition(aRoomPosition)
    local rect = self:rectangle()
    local pt = aRoomPosition:asScreenPoint()
    if rect:containsPoint(pt) then
        local newRP = RoomPosition:fromScreenLocation(self, x,y)
        return true, newRP
    else
        return false,aRoomPosition
    end
end

x and y aren’t defined above. We had them as locals in the original, fetched from aRoomPosition. This isn’t really what we ought to be doing.

This will work:

function Room:acceptRoomPosition(aRoomPosition)
    local rect = self:rectangle()
    local pt = aRoomPosition:asScreenPoint()
    if rect:containsPoint(pt) then
        local newRP = RoomPosition:fromScreenLocation(self, pt.x,pt.y)
        return true, newRP
    else
        return false,aRoomPosition
    end
end

But this is another Demeter violation, ripping the guts out of our pt. Instead we want:

function Room:acceptRoomPosition(aRoomPosition)
    local rect = self:rectangle()
    local pt = aRoomPosition:asScreenPoint()
    if rect:containsPoint(pt) then
        local newRP = RoomPosition:fromScreenPoint(self, pt)
        return true, newRP
    else
        return false,aRoomPosition
    end
end

And RoomPosition needs to know how to do that:

function RoomPosition:fromScreenPoint(aRoom, aPoint)
    return self:fromScreenLocation(aRoom, aPoint.x, aPoint.y)
end

But now RoomPosition is violating Demeter. So:

function RoomPosition:fromScreenPoint(aRoom, aPoint)
    local x,y = aPoint:xy()
    return self:fromScreenLocation(aRoom, x, y)
end

function Point:xy()
    return self.x, self.y
end

But I still don’t love this. RoomPosition shouldn’t have to pull the point apart at all. The real answer, I think, is that RoomPosition shouldn’t have x and y at all. It should have a point.

We’re green right now, let’s commit and then try that.

Commit: refactoring toward Point object.

Now let’s look at RoomPosition …

Let’s begin by just changing the internals. I expect some folks are ripping them out. We’ll find out:

function RoomPosition:init(aRoom, localX, localY)
    self.room = aRoom
    self.localPos = Point(localX,localY)
end

I expect I’ll want to be passed a Point here but not yet.

Fixing internals:

function RoomPosition:__tostring()
    return string.format("RP %d (%f,%f)", self.room.number, self.point:__tostring())
end

I have some concern about that one …

This:

function RoomPosition:__eq(aRoomPosition)
    return self.room == aRoomPosition.room and self.x == aRoomPosition.x and self.y == aRoomPosition.y
end

… wants to become:

function RoomPosition:__eq(aRoomPosition)
    return self.room == aRoomPosition.room and self.localPos == aRoomPosition.localPos
end

I’m gong to run now and see what explodes. A couple of tests break, plus this:

RoomPosition:46: attempt to perform arithmetic on a nil value (field 'x')
stack traceback:
	RoomPosition:46: in method 'screenLocation'
	Player:14: in method 'draw'
	Main:43: in function 'drawDungeonCreation'
	Main:32: in function 'draw'

Let’s fix that:

function RoomPosition:screenLocation()
    return self.room.x + self.x, self.room.y + self.y
end

Well this ought to be adding two points all day, but of course the caller wants the x and y. Still:

function RoomPosition:screenLocation()
    local pt = self.room:screenPoint() + self.localPos
    return pt.x,pt.y
end

function Room:screenPoint()
    return Point(self.x,self.y)
end

RoomPosition:51: attempt to perform arithmetic on a nil value (field 'x')
stack traceback:
	RoomPosition:51: in method 'move'
	Player:39: in method 'move'
	Player:31: in method 'keyDown'
	Main:25: in function 'keyboard'

function RoomPosition:move(deltaX,deltaY)
    local proposedX = self.x + deltaX
    local proposedY = self.y + deltaY
    return self.room:legalMoveTo(RoomPosition(self.room,proposedX,proposedY))
end

I’m not going to push the objects all the way down here, my mental stack is getting too deep.

function RoomPosition:move(deltaX,deltaY)
    local proposedX = self:localX() + deltaX
    local proposedY = self:localY() + deltaY
    return self.room:legalMoveTo(RoomPosition(self.room,proposedX,proposedY))
end

function RoomPosition:localX()
    return self.localPos.x
end

function RoomPosition:localY()
    return self.localPos.y
end

Still not loving the breakouts but for now, try the program:

Point:12: attempt to perform arithmetic on a nil value (field 'x')
stack traceback:
	Point:12: in metamethod '__add'
	RoomPosition:34: in method 'asScreenPoint'
	Room:131: in method 'acceptRoomPosition'
	Room:365: in function <Room:356>
	(...tail calls...)
	Player:39: in method 'move'
	Player:31: in method 'keyDown'
	Main:25: in function 'keyboard'

Ooo, error in our __add. Since that’s deep in the bag of tricks, it’s a yellow card if not a red one.

The asScreenPoint is:

function RoomPosition:asScreenPoint()
    return self.room:centerScreenPoint() + self:asLocalPoint()
end

When I look for those two methods, both seem to be returning points. Maybe my add doesn’t work. I reckon this calls for a test.

        _:test("Point addition", function()
            local p1 = Point(1,2)
            local p2 = Point(10,20)
            local pa = Point(11,22)
            local p3 = p1 + p2
            _:expect(p3).is(pa)
        end)

That works fine. So we must have passed in something weird to our add. Let’s find out.

This is giving me a failure in the _add.

function Room:acceptRoomPosition(aRoomPosition)
    local rect = self:rectangle()
    local pt = aRoomPosition:asScreenPoint()
    if rect:containsPoint(pt) then
        local newRP = RoomPosition:fromScreenPoint(self, pt)
        return true, newRP
    else
        return false,aRoomPosition
    end
end

I think it must be happening in or around fromScreenPoint:

When I move the princess, I get this:

~~~Point:7: point without x stack traceback: [C]: in function ‘assert’ Point:7: in field ‘init’ … false end

setmetatable(c, mt)
return c end:24: in function <... false
end

setmetatable(c, mt)
return c end:20>
(...tail calls...)
RoomPosition:34: in method 'asScreenPoint'
Room:131: in method 'acceptRoomPosition'
Room:366: in function <Room:357>
(...tail calls...)
Player:39: in method 'move'
Player:31: in method 'keyDown'
Main:25: in function 'keyboard' ~~~

So that is somewhat weird, isn’t it? Init looks like this:

function Point:init(x,y)
    assert(x, "point without x")
    assert(y, "point without y")
    self.x = x
    self.y = y
end

Because I put those asserts in there. So somehow asScreenPoint isn’t doing the job it should?

function RoomPosition:asScreenPoint()
    return self.room:centerScreenPoint() + self:asLocalPoint()
end

Maybe let’s print the values? That quickly tells me that asLocalPoint is sending bad values to Point creation. This alerts me, finally, to pay more attention:

function RoomPosition:asLocalPoint()
    return Point(self.x,self.y)
end

We don’t have x and y any more. We have self.localPos, which is a Point. Duh.

function RoomPosition:asLocalPoint()
    return self.localPos
end

That repaired, I get this:

attempt to compare number with nil
stack traceback:
	[C]: in function 'math.min'
	RoomPosition:72: in function 'clamp'
	RoomPosition:66: in function <RoomPosition:64>
	(...tail calls...)
	Player:39: in method 'move'
	Player:31: in method 'keyDown'
	Main:25: in function 'keyboard'

Yes, we’ve not done anything with that yet.

function RoomPosition:clamp()
    local lox,loy,hix,hiy = self.room:localCorners()
    local x = clamp(lox,self.x,hix)
    local y = clamp(loy,self.y,hiy)
    return RoomPosition(self.room,x,y)
end

Yes, self.x is nothing all right.

function RoomPosition:clamp()
    local lox,loy,hix,hiy = self.room:localCorners()
    local x = clamp(lox,self.localPos.x,hix)
    local y = clamp(loy,self.localPos.y,hiy)
    return RoomPosition(self.room,x,y)
end

This is a bit anti Demeter¹, but everything is now green (well, yellow, we have that ignored test) and the princess moves as before.

Commit: further refactoring to use Rectangle and Point.

I think we’ll call it a day at this point, with a bit of improvement. No, let’s see about doing this right:

function RoomPosition:clamp()
    local lox,loy,hix,hiy = self.room:localCorners()
    local x = clamp(lox,self.localPos.x,hix)
    local y = clamp(loy,self.localPos.y,hiy)
    return RoomPosition(self.room,x,y)
end

This seems to me to be a job for Rectangle and Point. How about this:

function RoomPosition:clamp()
    local pt = self.room:rectangle():clampPoint(self:asScreenPoint())
    return RoomPosition:fromScreenPoint(self.room, pt)
end

function Rectangle:clampPoint(aPoint)
    lox,loy,hix,hiy = self:corners()
    local x,y = aPoint:xy()
    local xn = clamp(lox,x,hix)
    local yn = clamp(loy,y,hiy)
    return Point(xn,yn)
end

function clamp(lo,val,hi)
    return math.max(math.min(val,hi), lo)
end

This works fine and, I think, gets the function more where it belongs. And I’m tired, so it’s time to break for the day, after perhaps just a bit of summing up.

Commit: further toward Rectangle and Point.

Summing Up

We’ve just done a bit of improvement, or attempted improvement, to our design. We’re moving in the direction of eliminating primitive obsession by having some low-level objects of our own that we can use to check ranges and the like.

We’re moving toward avoiding direct access to member variables of objects we have handles on, instead calling methods even if those methods do nothing more than return a member variable.

We’ve moving toward a more explicit differentiation between screen locations and room-local locations, but I think that’s still a bit problematical. It would be far better if there were just one kind of location, not two. That’s evidenced by our clamp method:

function RoomPosition:clamp()
    local pt = self.room:rectangle():clampPoint(self:asScreenPoint())
    return RoomPosition:fromScreenPoint(self.room, pt)
end

The room rectangle should probably be named ScreenRectangle, because that’s what it is. Rooms are going to be tricky, because they have a screen-oriented center, and are usually addressed with local positions offset from that center.

I’m thinking that this is a better approach, and that things will get incrementally better. I’m also thinking that some readers, if there are any, will be thinking that we’re adding objects and methods and conversions that aren’t much simpler from what we had, if they are simpler at all

We’ll see. See you next time!

D1.zip