BOTS Thursday
I think I’m on my own today. Feeling kind of lonely. I’ll see what I can figure out.
At the FGNO1, we discovered that the Bot can drop things off the edge of the world. We think it can also take them back, but that was harder to verify. Anyway, that’s something I could deal with.
I made some notes during FGNO:
- Things on top of things. (Nothing prevents two objects from being added on top of each other but (I think) we have prevented a bot dropping something on top of another thing.)
- Inventory is a list but we only ever hold one thing.
- Entity access via x, y is slow, we might want to do something about that.
- Clocking. (Especially in server-client, how can we ensure that each bot gets equal access to world services? Should we be doing something now?)
- Vision. (I have that weird idea about a visual pattern. Early tests were not as inspiring as I had hoped.)
- Logging (on screen, so that we can see what bots are thinking etc)
- Bot view on screen. (It would be interesting to be able to see what a bot knows about the world. presently our pygame stuff shows the world’s view.)
- Drop off screen! (See above.)
- Robot eats robot!?!? (We think we saw one robot take another when we had several running at once.)
My practice is to write notes down as I think of them. I make the notes brief but hopefully evocative. Then, having written them down, I generally go back to whatever I was working on. Writing them down serves two purposes: it makes a record that I can possibly refer to later to recover an idea, and it lets me drop the idea for the moment, so that I can focus on fewer things at once. I almost never divert what I’m doing to address a note that just came up, but if I bump into the issue again later, I might.
I confess that I haven’t tracked any statistics on how often I go back, how often the idea is useless, and so on. Confess it? I declare it.
Let’s look at the drop and take and the edge of the world. What even is the edge of the world?
class World:
def __init__(self, max_x, max_y):
self.width = max_x
self.height = max_y
self.map = Entities()
I think things are named that way so as to make it clear that when we create the world, say, 50, 50, we mean to include 0 through 50. But I’m not at all sure of that. Let’s see what really happens.
When we move an entity (presumably only bots move at present, but that could change) the code is this:
def _move(self, entity, dx, dy):
entity = self.map.contents[entity.id]
location = entity.location
new_x = self.clip(location.x + dx, self.width)
new_y = self.clip(location.y + dy, self.height)
entity.location = Point(new_x, new_y)
entity.vision = self.create_vision(entity.location)
def clip(self, coord, limit):
return 0 if coord < 0 else (limit if coord > limit else coord)
Yes. Clip checks for coordinate > limit, so if width is 50, x can be 50. I’m not sure that’s a good decision, but it is the decision we made. I think we’re going to find quite soon that we need that clipping code in other places. Here’s drop:
def drop(self, bot, entity):
location = bot.location + bot.direction
if not self.map.entity_at(location.x, location.y):
entity.location = location
self.add(entity)
bot.remove(entity)
We only drop “forward”, that is, in the direction the bot is facing. Clearly we are not checking the location for being in the world, and the math could produce a negative coordinate or one greater than the world size.
I can see at least two ways to go: we could simply clip the coordinate and use it. If it changes, whichever coordinate went off-world will be set back and the coordinate we get will be the same as the bot coordinates and there is a bot there and therefore we will find an entity and therefore we will not drop.
I think that works and is too clever. Also, I think we need a test.
def test_bot_cannot_drop_off_world(self):
world = World(10, 10)
bot = Bot(10, 5)
block = Block(4, 4)
bot.receive(block)
bot.direction = Direction.EAST
world.drop(bot, block)
assert bot.has(block), 'drop should not happen'
The test fails, no surprise there.
Let’s fix drop:
def drop(self, bot, entity):
drop_location = bot.location + bot.direction
if self.is_off_world(drop_location):
return
if not self.map.entity_at(drop_location.x, drop_location.y):
entity.location = drop_location
self.add(entity)
bot.remove(entity)
We need the new method is_off_world:
def is_off_world(self, location):
return self.clip(location.x, self.width) != location.x \
or self.clip(location.y, self.height) != location.y
Green. Commit: drop will not drop entities off world.
Reflection
So that seems nice to me. But how do you feel about the test that drove the result? I think it is clear that the is_off_world code handles all four edges. But that code was not “forced”, in order to make the tests run. In principle, I could have just checked the x coordinate for over the limit.
Do you think we should write the other three tests? I could go either way. Since it won’t take long, let’s do it.
def test_bot_cannot_drop_off_world_north(self):
world = World(10, 10)
bot = Bot(5, 10)
block = Block(4, 4)
bot.receive(block)
bot.direction = Direction.NORTH
world.drop(bot, block)
assert bot.has(block), 'drop should not happen'
def test_bot_cannot_drop_off_world_east(self):
world = World(10, 10)
bot = Bot(10, 5)
block = Block(4, 4)
bot.receive(block)
bot.direction = Direction.EAST
world.drop(bot, block)
assert bot.has(block), 'drop should not happen'
def test_bot_cannot_drop_off_world_south(self):
world = World(10, 10)
bot = Bot(5, 0)
block = Block(4, 4)
bot.receive(block)
bot.direction = Direction.SOUTH
world.drop(bot, block)
assert bot.has(block), 'drop should not happen'
def test_bot_cannot_drop_off_world_west(self):
world = World(10, 10)
bot = Bot(0, 5)
block = Block(4, 4)
bot.receive(block)
bot.direction = Direction.WEST
world.drop(bot, block)
assert bot.has(block), 'drop should not happen'
There we go. Commit: additional tests for off-world drop.
Reflection
Note in the tests that north is 10 and south is zero. That’s because:
Direction.NORTH = Direction(0, 1)
Direction.EAST = Direction(1, 0)
Direction.SOUTH = Direction(0, -1)
Direction.WEST = Direction(-1, 0)
Now as a human being, living in the northern hemisphere, in the year 2024, I think of north as up, and on a graph I think of y going up, so I think that north should increment the y coordinate. (Do not speak to me of geographic coordinates and parallels counting up to the south.)
However, pygame, like many games, thinks that the origin of the screen is on the top left, and thus y increases downward. This is going to lead to confusion. Indeed, when we were working at FGNO, it did cause some. Now, if it were up to me, I would make pygame adhere to my thinking, with origin at lower left like we used to do it in fourth grade or wherever we learned to draw graphs. It is not presently up to me, and we’ll see what happens.
I think that we should rename drop to drop_forward, which should make it a bit more clear what it is going to do. PyCharm does the refactoring for me. Commit: rename drop to drop_forward
Clearly we should do the same with take, which looks like this:
def take(self, bot: Bot):
location = bot.location + bot.direction
entity = self.map.entity_at(location.x, location.y)
if entity:
if entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
Also, and you’re going to want to stab me for this. I’m going to give it the same treatment as drop. First the rename this time. Done, commit.
Ah notice the check for not taking a robot. I guess we patched that in at some point. I’d wager there’s no test for it. I’d win the bet, because I just changed it to check for X and no tests broke.
Let’s do the coordinate check first. And then look at these two methods:
def take_forward(self, bot: Bot):
drop_location = bot.location + bot.direction
if self.is_off_world(drop_location):
return
entity = self.map.entity_at(drop_location.x, drop_location.y)
if entity:
if entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
def drop_forward(self, bot, entity):
drop_location = bot.location + bot.direction
if self.is_off_world(drop_location):
return
if not self.map.entity_at(drop_location.x, drop_location.y):
entity.location = drop_location
self.add(entity)
bot.remove(entity)
Make them more similar:
def take_forward(self, bot: Bot):
take_location = bot.location + bot.direction
if self.is_off_world(take_location):
return
entity = self.map.entity_at(take_location.x, take_location.y)
if entity:
if entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
def drop_forward(self, bot, entity):
drop_location = bot.location + bot.direction
if self.is_off_world(drop_location):
return
entity = self.map.entity_at(drop_location.x, drop_location.y)
if not entity:
entity.location = drop_location
self.add(entity)
bot.remove(entity)
Hm. What I had in mind here was something clever about turning those top four lines into a method and using it twice. I don’t quite see how to do that. Let’s see … could we return a valid location or None to useful effect here?
Arrgh that change above does not work, it shadows entity for the drop. Worse yet, the tests don’t fail!
Let’s roll back.
def take_forward(self, bot: Bot):
location = bot.location + bot.direction
entity = self.map.entity_at(location.x, location.y)
if entity:
if entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
def drop_forward(self, bot, entity):
drop_location = bot.location + bot.direction
if self.is_off_world(drop_location):
return
if not self.map.entity_at(drop_location.x, drop_location.y):
entity.location = drop_location
self.add(entity)
bot.remove(entity)
Let’s refactor the drop, by hand, to request a validated location, which can return None.
def drop_forward(self, bot, entity):
drop_location = self.validate_forward(bot)
if drop_location:
if not self.map.entity_at(drop_location.x, drop_location.y):
entity.location = drop_location
self.add(entity)
bot.remove(entity)
def validate_forward(self, bot):
location = bot.location + bot.direction
return None if self.is_off_world(location) else location
def is_off_world(self, location):
return self.clip(location.x, self.width) != location.x \
or self.clip(location.y, self.height) != location.y
Ah, that’s not bad. And is this better?
def drop_forward(self, bot, entity):
drop_location = self.validate_forward(bot)
if drop_location and not self.map.entity_at(drop_location.x, drop_location.y):
entity.location = drop_location
self.add(entity)
bot.remove(entity)
And is this better?
def drop_forward(self, bot, entity):
drop_location = self.validate_forward(bot)
if drop_location and self.is_empty(drop_location):
entity.location = drop_location
self.add(entity)
bot.remove(entity)
def is_empty(self, drop_location):
return not self.map.entity_at(drop_location.x, drop_location.y)
I think I rather like this. Let’s rename the location:
def drop_forward(self, bot, entity):
valid_location = self.validate_forward(bot)
if valid_location and self.is_empty(valid_location):
entity.location = valid_location
self.add(entity)
bot.remove(entity)
def is_empty(self, drop_location):
return not self.map.entity_at(drop_location.x, drop_location.y)
def validate_forward(self, bot):
location = bot.location + bot.direction
return None if self.is_off_world(location) else location
def is_off_world(self, location):
return self.clip(location.x, self.width) != location.x \
or self.clip(location.y, self.height) != location.y
Commit that and then think. Commit: refactoring.
Reflection
There is one thing about this that I do not like. It is that valid_location can be None, but seems like a location, so one might accidentally use it and wind up stuffing it into an entity, and That Would Be Bad™.
I’m not sure what to do about that. Renaming the variable might help?
Ah, what if we return the original location if the proposed one is not valid:
def drop_forward(self, bot, entity):
valid_location = self.validate_forward(bot)
if valid_location != bot.location and self.is_empty(valid_location):
entity.location = valid_location
self.add(entity)
bot.remove(entity)
def validate_forward(self, bot):
location = bot.location + bot.direction
return bot.location if self.is_off_world(location) else location
I think we’ll go with this. Commit: refactoring.
Now let’s go after take. I’ve made a note that we need better drop and take tests.
We start here:
def take_forward(self, bot: Bot):
location = bot.location + bot.direction
entity = self.map.entity_at(location.x, location.y)
if entity:
if entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
We use our valid thingie:
def take_forward(self, bot: Bot):
valid_location = self.validate_forward(bot)
entity = self.map.entity_at(valid_location.x, valid_location.y)
if entity:
if entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
I think the check for ‘R’ should be more robust … let’s write out some conditions longhand:
def take_forward(self, bot: Bot):
valid_location = self.validate_forward(bot)
if valid_location == bot.location:
return
entity = self.map.entity_at(valid_location.x, valid_location.y)
if entity and entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
And then that check for ‘R’ is really trying to see if it is legit to take the thing in front of us:
def take_forward(self, bot: Bot):
valid_location = self.validate_forward(bot)
if valid_location == bot.location:
return
entity = self.map.entity_at(valid_location.x, valid_location.y)
if entity and self.bot_can_take(entity):
self.map.remove(entity.id)
bot.receive(entity)
def bot_can_take(self, entity):
return entity.name != 'R'
We can do better. Undo that.
def take_forward(self, bot: Bot):
valid_location = self.validate_forward(bot)
if valid_location == bot.location:
return
entity = self.map.entity_at(valid_location.x, valid_location.y)
if entity and entity.name != 'R':
self.map.remove(entity.id)
bot.receive(entity)
Inline. No, didn’t work.
def take_forward(self, bot: Bot):
valid_location = self.validate_forward(bot)
if valid_location == bot.location:
return
entity = self.map.entity_at(valid_location.x, valid_location.y)
if self.can_take_entity(entity):
self.map.remove(entity.id)
bot.receive(entity)
def can_take_entity(self, entity):
return entity and entity.name != 'R'
OK, I think I like that. Commit: refactoring take_forward.
Let’s review this code and see if there are a few more improvements before we set it down.
def take_forward(self, bot: Bot):
valid_location = self.validate_forward(bot)
if valid_location == bot.location:
return
entity = self.map.entity_at(valid_location.x, valid_location.y)
if self.can_take_entity(entity):
self.map.remove(entity.id)
bot.receive(entity)
def can_take_entity(self, entity):
return entity and entity.name != 'R'
def drop_forward(self, bot, entity):
valid_location = self.validate_forward(bot)
if valid_location != bot.location and self.is_empty(valid_location):
entity.location = valid_location
self.add(entity)
bot.remove(entity)
def is_empty(self, drop_location):
return not self.map.entity_at(drop_location.x, drop_location.y)
def validate_forward(self, bot):
location = bot.location + bot.direction
return bot.location if self.is_off_world(location) else location
def is_off_world(self, location):
return self.clip(location.x, self.width) != location.x \
or self.clip(location.y, self.height) != location.y
the name validate_forward isn’t quite right. First of all, a validate method sounds to me like one that returns true or false. Second, the name doesn’t express what it does, which is to return the bot’s forward location if it exists and otherwise the bot’s location. What is that?
Well, recall that if we were to try to move forward from the edge, the bot’s location would not change, it would remain set to the current value. So … is this method bots_next_location? Oh. Is it bots_forward_location, which is always bot+1, as it were, or bot, depending on if you’re looking over the edge of the world?
Let’s try that.
def take_forward(self, bot: Bot):
take_location = self.bots_next_location(bot)
if take_location == bot.location:
return
entity = self.map.entity_at(take_location.x, take_location.y)
if self.can_take_entity(entity):
self.map.remove(entity.id)
bot.receive(entity)
def drop_forward(self, bot, entity):
drop_location = self.bots_next_location(bot)
if drop_location != bot.location and self.is_empty(drop_location):
entity.location = drop_location
self.add(entity)
bot.remove(entity)
def bots_next_location(self, bot):
location = bot.location + bot.direction
return bot.location if self.is_off_world(location) else location
I think that’s better. It would be better still if we could send a message to location asking whether it is off-world or not. Then that last line could read:
return bot.location if location.is_off_world() else location
That would be nice. Maybe later, it is time for me to finish up here. Commit: refactoring.
Let’s sum up.
Added in Post
I noticed this code while reviewing the article:
def _move(self, entity, dx, dy):
entity = self.map.contents[entity.id]
location = entity.location
new_x = self.clip(location.x + dx, self.width)
new_y = self.clip(location.y + dy, self.height)
entity.location = Point(new_x, new_y)
entity.vision = self.create_vision(entity.location)
That can be changed, now, to:
def _move(self, entity, dx, dy):
entity = self.map.contents[entity.id]
entity.location = self.bots_next_location(entity)
entity.vision = self.create_vision(entity.location)
That’s why we do those little methods, right there. Sweet!
Summary
Just shy of two hours work. I think we have improved take and drop substantially, including some new tests. We have removed much of the duplication between the two methods, and I think—though we have not yet seen—that we’ll find some of those new methods useful elsewhere.
One lesson here: sometimes it takes multiple tries to find a way of expressing things that we actually prefer to the longhand. In my view, the small-methods style is always preferable, but it’s important that the methods make sense in context. I believe it’s better to try four times and find a decent refactoring than it is to try once, or zero times, and give up. I could be wrong.2
I think we should get serious about naming private methods in world with leading underbars, because in the future we’ll want to have just a few methods that can really be called by our Bot instances.
It does seem odd to me that I’ve consumed two hours on just these two methods, although there are also well over 500 lines in this article, and it does take time to write those as well.
Anyway the code is a bit better by my lights, so I think I’ll make a chai (well past time) and enjoy a melon.
See you next time!