BOTS Refactoring
I’m planning to pair with GeePaw this morning. I have some improvements in mind, and will start on those before pairing time. We gather stones together. Well past time, you ask me.
Based on what GeePaw and I considered yesterday, I have a few changes in mind. The overall goal is to get responsibilities better allocated among objects, which should lead to tests that are more granular, easier to write, more direct, and perhaps even less repetitive while not saying almost the same thing over and over.
I have two things in mind:
- Change Point class to Location, with ability to add a Direction to a Location and provide a new Location. I think Location will be fairly simple, without much orientation toward this particular game.
- Replace the Entities class with a new class, Map, which is to be the map of the world. It will enforce some of the world guidelines, though it remains to be seen which ones. It will provide access to entities by id and by location.
- (Wrong about the two, as usual.) Probably we should tie Bot and Block together in a single class, Entity or something.
- Do all of the above incrementally, supported by tests old and new.
- Probably something I’ve forgotten.
- Almost certainly something that comes up along the way.
Let’s get started.
Tests are green. One thing that I’ve forgotten is that there are five tests marked skip. They are:
@pytest.mark.skip("needs to exist")
def test_location_plus_direction_math(self):
pass
@pytest.mark.skip("needs to exist")
def test_world_deals_with_location_at_edge(self):
pass
@pytest.mark.skip("needs to exist")
def test_bot_gets_updated(self):
pass
@pytest.mark.skip("later")
def test_draw_world_with_bot(self):
@pytest.mark.skip("later")
def test_draw_world_with_second_entity(self):
Those last two are the ones that test drawing a typewriter picture of the world. They can probably be removed.
The first three look like a really good place to start today’s work. How nice!
We’ll begin with Point to Location, which should be fairly straightforward. Here’s Point:
class Point:
def __init__(self, x, y):
self.x = x
self.y = y
def __eq__(self, other):
return self.x == other.x and self.y == other.y
def __repr__(self):
return f'Point({self.x}, {self.y})'
def __add__(self, other):
return Point(self.x + other.x, self.y + other.y)
def distance(self, other):
return abs(self.x - other.x) + abs(self.y - other.y)
def step_toward(self, t):
dx = t.x - self.x
dy = t.y - self.y
if abs(dx) > abs(dy):
step = 1 if dx > 0 else -1
return Point(self.x + step, self.y)
else:
step = 1 if dy > 0 else -1
return Point(self.x, self.y + step)
Let’s begin by renaming Point to Location. PyCharm conveniently renames the Python file for us. Green. Commit: rename Point to Location. That modified 9 files for us, keeping the tests green. I do like those JetBrains folks.
Currently we have an add method in Location, as we can see above. And we have this empty test:
@pytest.mark.skip("needs to exist")
def test_location_plus_direction_math(self):
pass
I think we can manage a bit of a test here.
def test_location_plus_direction_math(self):
location = Location(5, 5)
assert location + Direction.NORTH == Location(5, 4)
assert location + Direction.SOUTH == Location(5, 6)
assert location + Direction.EAST == Location(4, 5)
assert location + Direction.WEST == Location(6, 5)
That passes, no surprise. I think I would like to require that the other in Location __add__ should only be a Direction. Let’s see what breaks if we do this:
class Location:
def __add__(self, other):
assert isinstance(other, Location)
return Location(self.x + other.x, self.y + other.y)
Twenty-one (21) tests fail. I am not surprised. Let’s see how many reasons there are.
OK the first reason is that I said Location up there and should have said Direction. I accepted PyCharm’s guess without thinking. Bad Ron, no biscuit. Now only five tests fail. Good work, Ron, have a banana.
Wait, what? My new test is one of the failing ones. What in the everlasting DELETED has happened? I don’t even have a clue.
I should roll back. I want to know what happened. Oh. It was always failing after I did east and west and I didn’t notice the popup. Test should be:
def test_location_plus_direction_math(self):
location = Location(5, 5)
assert location + Direction.NORTH == Location(5, 4)
assert location + Direction.SOUTH == Location(5, 6)
assert location + Direction.EAST == Location(6, 5)
assert location + Direction.WEST == Location(4, 5)
I should give back the banana. What a tiny fool.
OK the others are probably adding Location to Location and shouldn’t. Let’s see. Looks like one issue is in Vision:
class Vision:
def matches(self, pattern, location: Location):
index = 0
for direction in Direction.EVERY:
check_location = Location(direction.x, direction.y) + location
item = self.vision_at(check_location.x, check_location.y)
pattern_item = pattern[index]
if pattern_item != '?' and pattern_item != item:
return False
index += 1
return True
Yes, as anticipated, that should use Direction, not Location. We can use it directly:
def matches(self, pattern, location: Location):
index = 0
for direction in Direction.EVERY:
check_location = location + direction
item = self.vision_at(check_location.x, check_location.y)
pattern_item = pattern[index]
if pattern_item != '?' and pattern_item != item:
return False
index += 1
return True
All tests are green. I have to watch the game display before committing. It’s a sin, but I want to. Works fine. Commit: Ensure only Directions are added to Locations.
Let’s reflect.
Reflection
We presently have an assert in the Location’s add method. That really won’t do: we cannot crash the program whenever someone mistakenly adds something they shouldn’t. We need to have an overall strategy for errors that doesn’t include crashing the server. In this case, I think we should return a location, probably the input location. So:
def __add__(self, other):
try:
assert isinstance(other, Direction)
except AssertionError:
return self
new_location = Location(self.x + other.x, self.y + other.y)
return new_location
That’ll do. Commit: Location add returns self if not given a Direction
I should have a test for that. OK …
def test_location_add_returns_self_if_not_given_direction(self):
location = Location(5, 5)
assert location + location == location
Commit: new test.
What else? I think we’re good. GeePaw has not yet appeared. I’ll try the standard GeePaw! GeePaw! GeePaw! Nothing? Maybe bad connection. I will start on the map idea. No wait, what about those other skipped tests?
@pytest.mark.skip("needs to exist")
def test_world_deals_with_location_at_edge(self):
pass
@pytest.mark.skip("needs to exist")
def test_bot_gets_updated(self):
pass
I think those relate to the next moves. I propose to extend the Entities class to a Map class that manages entities and locations. It may be past time to create an Entity class relating to Bot and Block. They look like this:
class Block:
def __init__(self, x, y):
self.world = None
self.id = None
self.name = 'B'
self.location = Location(x, y)
class Bot:
def __init__(self, x, y, direction=Direction.EAST):
self.world = None
self.id = None
self.name = 'R'
self.location = Location(x, y)
self.direction = direction
self.direction_change_chance = 0.2
self.inventory = []
self._vision = None
self.tired = 10
self.state = "walking"
I think we might be wise to extract a superclass here, with the world, id, name, and location. I think I’ll wait on that until a pair shows up, if I can wait that long. We’ll live with the duck typing for now.
Let’s rename Entities class to Map, if no one objects. So far so good. Commit: rename Entities to Map
I’m thinking along these lines:
- Map will maintain rapid access to entities by Location or id.
- Only one Entity can reside at any given Location. (Subject to later change.)
- Map will support the boundaries of the world. (A map’s got to know its limitations.1)
- Map will provide a move capability that can succeed or fail based on boundaries and blockages.
As such, some operations now handled in World will move to Map. I wish I had a pair for this, but for now, I have not.
Let’s take this test:
@pytest.mark.skip("needs to exist")
def test_world_deals_with_location_at_edge(self):
pass
And focus it on the Map. Well, we’ll save this one in case it’s a good idea to test World and we’ll test Map directly. There is one somewhat meaningful test.
Ah, Hill’s here. We pair to some good effect.
Our main thing was to improve the Entities class, now renamed to Map, to provide more capability, as described above.
We wrote these two tests, one at a time, and made them work:
def test_map_successful_move(self):
map = Map(10, 10)
bot = Bot(5,5)
map.place(bot)
map.attempt_move(bot.id, Location(6,6))
def test_map_unsuccessful_move(self):
map = Map(10, 10)
bot = Bot(5,5)
map.place(bot)
map.attempt_move(bot.id, Location(10, 11))
assert bot.location == Location(5, 5)
Hill came up with the name attempt_move, which made sense to me. It goes like this:
class Map:
def __init__(self, width, height):
self.width = width
self.height = height
self.contents = {}
def attempt_move(self, id, location: Location):
entity = self.contents[id]
if self.location_is_valid(location):
entity.location = location
def location_is_valid(self, location: Location) -> bool:
return location.x >= 0 and location.x <= self.width and location.y >= 0 and location.y <= self.height
Note that we only check to be sure that we are moving within the world. We do not care how far the entity has moved: that’s up to the World, we think. We do not do the move if it takes us outside the width and height, which are now known to the Map object.
We removed a number of tests for bot motion and wound up with this:
class TestBot:
def test_step(self):
world = World(10, 10)
bot = Bot(5, 5)
world.add(bot)
location = bot.location
bot.direction = Direction.NORTH
bot.step()
assert bot.location == location + Direction.NORTH
Since we are sure that location arithmetic works and that Map won’t take us off world, we only need one test to be sure that bot.step() correctly applies direction and moves appropriately. I suppose in principle we could have a bug such that you can only move NORTH but seriously, we think this is enough.
Finally, in World, we replaced this:
def _move(self, entity):
entity = self.map.contents[entity.id]
entity.location = self.bots_next_location(entity)
entity.vision = self.create_vision(entity.location)
def step(self, bot):
self._move(bot)
With this:
def step(self, bot):
location = self.bots_next_location(bot)
self.map.attempt_move(bot.id, location)
bot.vision = self.create_vision(bot.location)
Small changes, but in a good direction. Probably a new reduction in code, mostly due to removing about four largish story tests.
Enough for today. See you next time!
Oh … I ran the game for a long time to see about clumping and look what happened. The bot gathers blocks and, since it will only drop a block next to another one, it tends to gather stones together. How very nice!
-
Harold Francis Callahan, private communication. ↩