Scanning BOTS Code
I think I’m on my own this morning. Maybe I should just open my Zoom to the public. Anyway, today let’s just scan the code and see what we can see and why we see it. Maybe even do something about it.
I’ll proceed in alphabetical order until I don’t. Here’s the Block:
class Block:
def __init__(self, x, y):
self.world = None
self.id = None
self.name = 'B'
self.location = Location(x, y)
@property
def x(self):
return self.location.x
@property
def y(self):
return self.location.y
def is_close_enough(self, entity):
pe = entity.location
se = self.location
d = pe.distance(se)
return d < 10
My first thought about this class isn’t in the code. I am aware that there is also a class Bot, coming right up on the jukebox, with the same top structure, plus more. They are duck-typed, belonging to the presently imaginary superclass of all the things that can be in the world. We might do well to formalize that relationship with a real superclass.
The x and y properties raise a small concern in my mind. We have the object Location, representing a world position, and the object Direction, representing—wait for it—a direction. If we also need to break out x and y … that suggests to me that maybe some abstraction somewhere is missing some capability. I’ll see if a quick glance at senders brings anything to mine.
As you’d expect, it’s hard to identify all the senders. There are a lot of them in files that I do not recognize as being part of our app. There seem to be thing in venv that should not be there, part of pygame. We had some odd things happen with Git, which may relate. I’ve made a note to look into that. I think I’ll try removing these properties for a moment to see what breaks.
Running the tests before removing those, I find that two are already broken. This is bad, there are broken tests in the repo. This fails:
def test_bot_facing_north_block(self):
map = Map(10, 10)
bot = Bot(5, 5)
bot.direction = Direction.NORTH
map.place(bot)
bot.vision = map.create_vision(bot.location)
assert not bot.facing_block()
map.place(Block(5,4))
bot.vision = map.create_vision(bot.location)
assert bot.facing_block()
I try to settle down, not to panic. The sky will not fall. The first assert is failing. It thinks it is facing a block. I did tweak that yesterday: did I not notice a broken test … might have, I was running the game … anyway:
def facing_block(self):
look_at = self.location + self.direction
name = self.vision.name_at(look_at.x, look_at.y)
look_at_left = self.location + (self.direction + self.direction.left())
left_name = self.vision.name_at(look_at_left.x, look_at_left.y)
look_at_right = self.location + (self.direction + self.direction.right())
right_name = self.vision.name_at(look_at_right.x, look_at_right.y)
return name == 'B' and left_name == '_' or right_name == '_'
There need to be parens in there.
return name == 'B' and (left_name == '_' or right_name == '_')
Green. Commit: fix defect in facing_block.
I think there is a PyCharm feature about running the tests before a push. Maybe I should look into that.
OK, now let’s see what happens when we remove those properties. They are in fact used, in creating the “vision” and “scan” objects, which are both, by design, simple tuples with the entity “name”, x, and y. I could argue that any object with a location might also be said to have an x and y, and that providing both location and x,y makes some sense. I could also argue that sticking to location and unwinding it if you need to would be simpler.
I will add that notion to the notes.md file.
Now there is the method is_close_enough. That has to do with scan, which is not even in use as far as I know. There are some tests for it, so we’ll let it go.
Reflection
This is a bit more than a simple scan, where you might just fuzz your eyes and look at the shape of things. I generally do not do that consciously but watch this as we move on to Bot:
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 = self.walking
Oof! What is that, ten lines in the init. Ten member variables is way more than any reasonable object should have and surely indicates that it needs splitting. I also notice that single underbarred member. Odd, though I know why it’s like that.
It’s like that because … well, this is probably not a great reason.
The World sets the Bot’s vision:
class World:
def step(self, bot):
location = self.bots_next_location(bot)
self.map.attempt_move(bot.id, location)
bot.vision = self.map.create_vision(bot.location)
The Bot does not want a raw vision list, which is just a list of tuples. It wraps the vision on receipt:
class Bot:
@property
def vision(self):
return self._vision
@vision.setter
def vision(self, vision):
self._vision = Vision(vision)
The Vision object is the clever object that recognizes patterns in the surrounding objects, used in the drop logic.
A larger issue comes to mind. As a programmer on the project, and because Hill mentioned it just yesterday, I know that we seem to have at least two ways of passing things between World and Bot, and we should settle on one. Note made.
Scanning on down my loose scan picks up this patch of code:
class Bot:
def do_something(self):
self.state()
self.move()
def laden(self):
if self.tired <= 0:
if self.near_block():
block = self.inventory[0]
self.world.drop_forward(self, block)
if block not in self.inventory:
self.tired = 5
self.state = self.walking
def looking(self):
if self.facing_block():
self.take()
if self.inventory:
self.tired = 5
self.state = self.laden
def walking(self):
if self.tired <= 0:
self.state = self.looking
This is the elementary state machine that operates the bot. Three states, and in order they really should be walking, looking, and laden, because that’s the order they occur in.
Let’s reorder those three. Commit: tidying.
I think the state machine could be and should be extracted into a separate class.
Then I see two adjacent large methods, but decide to focus on them one at a time:
def facing_block(self):
look_at = self.location + self.direction
name = self.vision.name_at(look_at.x, look_at.y)
look_at_left = self.location + (self.direction + self.direction.left())
left_name = self.vision.name_at(look_at_left.x, look_at_left.y)
look_at_right = self.location + (self.direction + self.direction.right())
right_name = self.vision.name_at(look_at_right.x, look_at_right.y)
return name == 'B' and (left_name == '_' or right_name == '_')
I wrote this yesterday, so that I know what it is doing. Hill and I agreed that we wanted the Bot to be so dull that it would only notice a block if it was right in front of the Bot. The Vision pattern logic can’t currently handle this situation very well—take my word for it—so instead we began with just this:
def facing_block(self):
look_at = self.location + self.direction
name = self.vision.name_at(look_at.x, look_at.y)
return name == 'B'
The location to look at is the one right in front of the Bot, i.e. its location plus its direction. So that part was easy.
Last night, while playing, I added an additional check, such that the block in question should have at least one side where there was nothing else. So I added the additional checks, which are a bit obscure to say the least.
I think this needs improvement. It needs it badly enough that we’ll do it now.
What if Location could be more helpful, with some handy methods like forward, forward_left, forward_right. We could write this (Wishful Thinking, or By Intention):
First step, rename the temps:
def facing_block(self):
forward = self.location + self.direction
name = self.vision.name_at(forward.x, forward.y)
forward_left = self.location + (self.direction + self.direction.left())
left_name = self.vision.name_at(forward_left.x, forward_left.y)
forward_right = self.location + (self.direction + self.direction.right())
right_name = self.vision.name_at(forward_right.x, forward_right.y)
return name == 'B' and (left_name == '_' or right_name == '_')
Next step, call the function we want.
class Bot:
def facing_block(self):
forward = self.location.forward(self.direction)
...
class Location:
def forward(self, direction):
return self + direction
Green. Do the next one:
class Bot:
def facing_block(self):
forward = self.location.forward(self.direction)
name = self.vision.name_at(forward.x, forward.y)
forward_left = self.location.forward_left(self.direction)
...
class Location:
def forward_left(self, direction):
return self + direction + direction.left()
Green. Do the third:
class Bot:
def facing_block(self):
forward = self.location.forward(self.direction)
name = self.vision.name_at(forward.x, forward.y)
forward_left = self.location.forward_left(self.direction)
left_name = self.vision.name_at(forward_left.x, forward_left.y)
forward_right = self.location.forward_right(self.direction)
right_name = self.vision.name_at(forward_right.x, forward_right.y)
return name == 'B' and (left_name == '_' or right_name == '_')
class Location:
def forward_right(self, direction):
return self + direction + direction.right()
Green. Let’s commit this much: refactoring.
Now I really hate the name_at for requiring x and y, since I do not have them to hand. I say this:
def facing_block(self):
forward = self.location.forward(self.direction)
name = self.vision.name_at(forward.x, forward.y)
forward_left = self.location.forward_left(self.direction)
left_name = self.vision.name_at(forward_left)
forward_right = self.location.forward_right(self.direction)
right_name = self.vision.name_at(forward_right)
return name == 'B' and (left_name == '_' or right_name == '_')
I am asking the vision.name_at to accept a Location. I implement this somewhat atrocious method:
def name_at(self, x, y=None):
if isinstance(x, Location):
xx = x.x
yy = x.y
else:
xx = x
yy = y
for name, vx, vy in self.vision_list:
if vx == xx and vy == yy:
return name
return '_'
We are green but that is atrocious. I could perhaps use the new(ish?) overloading but a quick trial didn’t go well.
Instead let’s do name_at this way:
def name_at(self, x, y=None):
if isinstance(x, Location):
xx = x.x
yy = x.y
else:
xx = x
yy = y
return self.find_name_at(xx, yy)
def find_name_at(self, xx, yy):
for name, vx, vy in self.vision_list:
if vx == xx and vy == yy:
return name
return '_'
And then …
def name_at(self, x, y=None):
if isinstance(x, Location):
return self.find_name_at(x.x, x.y)
else:
return self.find_name_at(x, y)
def find_name_at(self, xx, yy):
for name, vx, vy in self.vision_list:
if vx == xx and vy == yy:
return name
return '_'
Nearly good. Commit: refactoring.
A type hint might help in the name_at.
#overload
def name_at(self, x: Union[Location, float], y: Union[float, None]=None):
if isinstance(x, Location):
return self.find_name_at(x.x, x.y)
else:
return self.find_name_at(x, y)
I think that’s an improvement. Commit: refactoring.
One thing that would be better still would be if everyone sent a location to name_at. Let’s see if that’s easy.
LOL, there are only two. One of them is the one I forgot in my facing_block method. The other is in Vision class. So I change those and then this:
def name_at(self, location: Location):
return self._find_name_at(location.x, location.y)
def _find_name_at(self, x, y):
for name, vx, vy in self.vision_list:
if vx == x and vy == y:
return name
return '_'
I decided to leave the search broken out. If I inline, I either have to break out x and y as temps or refer to location.x and location.y in the loop. This is, marginally, better, I think. Today.
Commit: refactoring, much simpler, finally.
I could continue but arguably this article is seriously overweight, so let’s glance at the other method I wanted to improve, and then sum up. Following the facing_block method, we have this:
def near_block(self):
p1 = 'B_???????'
if self.vision.matches(p1, self.location):
self.direction = Direction.NORTH
return True
if self.vision.matches('__B??????', self.location):
self.direction = Direction.NORTH
return True
if self.vision.matches('BBB??????', self.location):
self.direction = Direction.WEST
return True
return False
There is question among the troops as to whether we can legitimately recognize something other than right in front of us, which somewhat argues against improving this until that matter is settled. That aside, we can imagine doing this in some kind of loop over a pattern and direction pair. We’ll leave this for another session.
Summary
A slip-up yesterday caused me to commit two broken tests. After a few moments of panic, I quickly fixed the issue and re-pushed. It does seem that if there’s a PyCharm feature about running the tests before a push, it would be a good thing to turn on.
Curiously, after fixing the defect, I didn’t really review the method, facing_block, until we returned to it later. Instead, like an ant, I went back to my previously-assigned task, inspecting the Block class. Arguably I should have taken the occasion to review facing_block right then, since it had shown itself to be problematical. I don’t think I even considered that. If I had, I might still have backed away slowly, since it had just broken. Anyway, we fixed it all up later.
Well, somewhat. Here it is now:
def facing_block(self):
forward = self.location.forward(self.direction)
name = self.vision.name_at(forward)
forward_left = self.location.forward_left(self.direction)
left_name = self.vision.name_at(forward_left)
forward_right = self.location.forward_right(self.direction)
right_name = self.vision.name_at(forward_right)
return name == 'B' and (left_name == '_' or right_name == '_')
I’m not sure we can let this stand. There are clearly three things happening here, and then the return. Let’s extract like this:
def facing_block(self):
name = self.forward_name()
left_name = self.forward_left_name()
right_name = self.forward_right_name()
return name == 'B' and (left_name == '_' or right_name == '_')
def forward_right_name(self):
forward_right = self.location.forward_right(self.direction)
right_name = self.vision.name_at(forward_right)
return right_name
def forward_left_name(self):
forward_left = self.location.forward_left(self.direction)
left_name = self.vision.name_at(forward_left)
return left_name
def forward_name(self):
forward = self.location.forward(self.direction)
name = self.vision.name_at(forward)
return name
Now some inline:
def facing_block(self):
return self.forward_name() == 'B' and (self.forward_left_name() == '_' or self.forward_right_name() == '_')
def forward_name(self):
forward = self.location.forward(self.direction)
return self.vision.name_at(forward)
def forward_left_name(self):
forward_left = self.location.forward_left(self.direction)
return self.vision.name_at(forward_left)
def forward_right_name(self):
forward_right = self.location.forward_right(self.direction)
return self.vision.name_at(forward_right)
Commit: delicious refactoring. I call that better. Do you? Toot me up, if you like.
Where was I? Oh, yes, summing up.
We spotted our state machine, noted that it might benefit from an extract (which would help with Bot’s member variable problem by the way), left that for another day but tidied up the code a bit.
Then we went into a rather long cycle of refactoring facing_block, including improvements to Vision and Location objects, asking them to take on responsibilities that seem more to belong to them than to Bot.
We might have spotted those needs differently. When we see code like location.x it is a hint that we may be dealing with something that Location class could help with. We found that to be the case here, adding three little methods to Location, which helped Vision do its part of the job for Bot.
We went the long way to get Vision to its current refined state. I chose to overload the name_at method to accept, optionally, a location or an x, y pair. In retrospect, a quick check would have told me just to convert to location. So I took three or four steps when I might have taken fewer. No big deal, we see what we see, and so long as we’re taking small sensible steps, we’ll generally wind up in a better place. In my case, it took seeing the overloaded method to convince me not to do it. It’s like that sometimes.
Overall, a pleasant session, no big issues, tiny steps, lots of commits (and even more were possible). I am pleased with the morning’s work.
See you next time!