Class per State (3)
This morning, tests for the final state class, Walking, then create the class and plug it in. Then, time permitting, I think we can simplify things a bit. And wow, can we! Early bird, meet worm. Used “rip-roarin’” in a sentence.
The state classes all support two methods, update and action. The two done yesterday, Looking and Laden, each have two cases in each method, and thus got four little tests each. I think that Walking is the same, but we’ll look at its current code to see what we can see.
class Machine:
def walking_update(self):
if self._knowledge.tired <= 0:
if self._knowledge.has_block:
return None, None, Laden()
else:
return None, None, Looking()
return self.walking_states()
def walking_action(self):
return []
Oh nice, no intelligence at all in the action method. One less test to do. One fewer tests? Whatever. We’ll come back and look at the rest of the Machine class once this is done, with an eye to getting rid of most of it.
def test_walking_action_returns_empty_list(self):
state = Walking()
knowledge = FakeKnowledge()
assert state.action(knowledge) == []
This drives out a small amount of code:
class Walking:
def action(self, _knowledge):
return []
Could have done that in two steps, I suppose, but I was thinking about action not about just creating the class. Anyway commit: first Walking test passes.
So we need a test for update. Let’s use the “Fake It Til You Make It” pattern, just for fun. I see upon looking at the update method up there that we’ll need more than two tests because we are checking both tired and has_block. So be it.
def test_walking_tired_without_block_keeps_walking(self):
state = Walking()
knowledge = FakeKnowledge(tired=5, has_block=False)
u, a, c = state.update(None, knowledge)
assert u is None
assert a is None
assert isinstance(c, Walking)
PyCharm suggested the last two lines correctly. Nice. Test fails, of course but this will pass:
class Walking:
def update(self, machine, knowledge):
return None, None, Walking()
Passing. Commit: first update test.
Then this test:
def test_walking_tired_with_block_keeps_walking(self):
state = Walking()
knowledge = FakeKnowledge(tired=5, has_block=True)
u, a, c = state.update(None, knowledge)
assert u is None
assert a is None
assert isinstance(c, Walking)
Also passes, of course. Might as well commit: second update test passing.
Moving right along, a new test:
def test_walking_not_tired_with_block_goes_laden(self):
state = Walking()
knowledge = FakeKnowledge(tired=0, has_block=True)
u, a, c = state.update(None, knowledge)
assert u is None
assert a is None
assert isinstance(c, Laden)
This, of course, fails having still produced Walking:
> assert isinstance(c, Laden)
E assert False
E + where False = isinstance(<machine.Walking object at 0x101f1b470>, Laden)
Not surprising, given the fake implementation. So let’s continue to fake it, but now a little less fake because we can return Laden as needed:
def update(self, machine, knowledge):
if knowledge.tired <= 0:
return None, None, Laden()
return None, None, Walking()
Now of course I could have just typed in the whole thing, I’m a full-grown bang beat, bell ringin’, big haul, great go, neck-or-nothin’, rip roarin’, every time a bull’s eye programmer. But there is something pleasant and incremental and fun about putting in just enough code to pass the test, sort of pretending not to know what’s really needed. So we commit: first laden test passing (still fake).
One more test should do the job:
def test_walking_not_tired_without_block_goes_looking(self):
state = Walking()
knowledge = FakeKnowledge(tired=0, has_block=False)
u, a, c = state.update(None, knowledge)
assert u is None
assert a is None
assert isinstance(c, Looking)
Huh! Just as expected, that doesn’t work. Let’s finally finish the method:
def update(self, machine, knowledge):
if knowledge.tired <= 0:
if knowledge.has_block:
return None, None, Laden()
else:
return None, None, Looking()
return None, None, Walking()
Green. Commit: state class Walking tested and working.
Now we get to break some things, because we’re going to plug this class into Machine and that will break some of the tests that check for the old-style state. That is to be expected. We’ll see what else breaks that surprises me. Probably some tests that may not even be needed, but we’ll think about what to do when we find out what happens.
I think all we should have to do is to change the Machine init to start in the new Walking state:
class Machine:
def __init__(self, knowledge):
assert isinstance(knowledge, Knowledge)
self._knowledge = knowledge
self._update = self.walking_update
self._action = self.walking_action
self._state = None
I am sure that we’ll break some tests when we do this, hopefully only ones looking for old-style walking.
class Machine:
def __init__(self, knowledge):
assert isinstance(knowledge, Knowledge)
self._knowledge = knowledge
self._update = None
self._action = None
self._state = Walking()
Three tests break. Here’s one, fixed:
def test_starts_walking(self):
bot = Bot(5, 5)
machine = Machine(bot._knowledge)
assert isinstance(machine._state, Walking)
I see two more places that need this change and fix them. Curiously, but this is actually good news, they break.
They are failing once changed, because our other two state classes are still returning the old-school form. Must think about that.
Also, this is distressing: I’ve changed the Looking and Laden to return the None, None, Walking() answer and some tests are broken.
Ah. The first one I look at reduces my distress, it is setting a test to the old style. I fix it:
def test_laden_goes_to_walking_if_no_block_in_inventory(self):
bot = Bot(5, 5)
machine = Machine(bot._knowledge)
machine.set_states((None, None, Laden()))
machine.state(bot._knowledge)
assert isinstance(machine._state, Walking)
I think I’ll search for senders of set_states and fix them in a batch.
All the changes are similar, none are at all surprising, just versions of needing to stop checking the _update and _action returns for a method and checking the _state return for being the right type.
I am green and can commit, but first I want to run the game, partly to see it and partly because I’m not 100% certain. It works as advertised. Commit: machine now uses only the state classes, none of its own state methods.
Now we can begin to unwind the Machine class, because most of its mechanism is now unused (except perhaps in some tests that we’ll want to think about a little bit but will probably decide are covered by our new ones). Here’s Machine now:
class Machine:
def __init__(self, knowledge):
assert isinstance(knowledge, Knowledge)
self._knowledge = knowledge
self._update = None
self._action = None
self._state = Walking()
def state(self, knowledge):
assert isinstance(knowledge, Knowledge)
self._knowledge = knowledge
knowledge.tired -= 1
if self._state:
info = self._state.update(self, self._knowledge)
self._update, self._action, self._state = info
else:
info = self._update()
self._update, self._action, self._state = info
if self._state:
return self._state.action(self._knowledge)
else:
return self._action()
def walking_states(self):
return self.walking_update, self.walking_action, None
def looking_states(self):
return None, None, Looking()
def laden_states(self):
return self.laden_update, self.laden_action, None
def set_states(self, states):
self._update, self._action, self._state = states
def walking_update(self):
if self._knowledge.tired <= 0:
if self._knowledge.has_block:
return None, None, Laden()
else:
return None, None, Looking()
return self.walking_states()
def walking_action(self):
return []
def looking_update(self):
if self._knowledge.has_block:
self._knowledge.tired = 5
return self.walking_states()
else:
return self.looking_states()
def looking_action(self):
if self._knowledge.can_take:
return ['take']
return []
def laden_update(self):
if not self._knowledge.has_block:
self._knowledge.tired = 5
return self.walking_states()
else:
return self.laden_states()
def laden_action(self):
if self._knowledge.tired <= 0:
if self._knowledge.can_drop:
return ['drop']
return []
I’ll just start deleting things. All these updates and actions should be unused. If they are used in tests, we should fix the tests or remove them. I’ll pull all six of those methods and see what explodes. Nothing explodes. Super.
I remove the state lists, but preserve set_states, which is used only in tests.
Now we have the state method, which we can pare way down:
class Machine:
def __init__(self, knowledge):
self._knowledge = knowledge
self._update = None
self._action = None
self._state = Walking()
def state(self, knowledge):
self._knowledge = knowledge
knowledge.tired -= 1
info = self._state.update(self, self._knowledge)
self._update, self._action, self._state = info
return self._state.action(self._knowledge)
def set_states(self, states):
self._update, self._action, self._state = states
Green. Commit: paring down Machine.
Now the _update and _action members are always None. We will break a lot of tests fixing that but let’s just get it over with. We change to this:
class Machine:
def __init__(self, knowledge):
self._knowledge = knowledge
self._state = Walking()
def state(self, knowledge):
self._knowledge = knowledge
knowledge.tired -= 1
info = self._state.update(self, self._knowledge)
self.set_states(info)
return self._state.action(self._knowledge)
def set_states(self, states):
self._update, self._action, self._state = states
No failures yet but this will do it:
def set_states(self, states):
_, _, self._state = states
Now we’re not setting those things that we check from time to time and a few tests break. For example:
def test_bot_notices_a_block(self):
world = World(10, 10)
bot = Bot(5, 5)
bot.state.tired = 0
bot.direction_change_chance = 0
world.add(bot)
block = Block(7, 5)
world.add(block)
world.set_bot_vision(bot)
bot.do_something()
assert bot.state._action == None # remove this
assert isinstance(bot.state._state, Looking)
bot.do_something()
assert bot.has(block)
We just need to stop checking that non-existent member. Three other tests are the same. All Fixed. Commit: adjust tests not to check _action and _update, no longer used.
Now there is no need for any of our objects to return those None, None bits. So in Machine:
def state(self, knowledge):
self._knowledge = knowledge
knowledge.tired -= 1
self._state = self._state.update(self, self._knowledge)
return self._state.action(self._knowledge)
We just expect one thing back. Lots of things break but we just go through all the update methods and fix their returns, like this, for example:
class Walking:
def update(self, machine, knowledge):
if knowledge.tired <= 0:
if knowledge.has_block:
return Laden()
else:
return Looking()
return Walking()
Lots of tests break, all because they are expecting a tuple back and don’t get one. Typical error:
> n1, n2, looking = state.update(machine, knowledge)
E TypeError: cannot unpack non-iterable Looking object
Fix is to remove the first two variables of the three. And there will be two checks for None after each one and those get removed. Done and done. Commit: state classes return just new state instance, not the old None, None.
All that remains now is to fix up set_states, which looks like this:
def set_states(self, states):
_, _, self._state = states
It’s only used four times, but let’s keep it because it covers a reference to a private member. Rename it to set_state.
No, it’s not worth its weight. Find the references and fix them.
Machine is down to this:
class Machine:
def __init__(self, knowledge):
self._knowledge = knowledge
self._state = Walking()
def state(self, knowledge):
self._knowledge = knowledge
knowledge.tired -= 1
self._state = self._state.update(self, self._knowledge)
return self._state.action(self._knowledge)
Commit: simplifying Machine.
I see no reason for machine to be retaining knowledge, it is passed to everyone as a parameter. Removing it breaks a few tests that check it but they are quickly revised to check bot._knowledge instead of machine._knowledge. Here’s Machine now:
class Machine:
def __init__(self):
self._state = Walking()
def state(self, knowledge):
knowledge.tired -= 1
self._state = self._state.update(self, knowledge)
return self._state.action(knowledge)
Commit: removed knowledge member.
I wonder if we can get rid of this class altogether. Here’s its use in Bot:
class Bot:
def __init__(self, x, y, direction=Direction.EAST):
...
self.state = Machine()
def do_something(self):
self.update()
actions = self.state.state(self._knowledge)
for action in actions:
match action:
case 'take':
self.world.take_forward(self)
case 'drop':
self.world.drop_forward(self, self.inventory[0])
case _:
assert 0, f'no case {action}'
self.move()
I changed that to push the logic up from Machine into Bot but noticed something in the game. I think the Bots are again missing chances to drop the Block.
No, on closer inspection, it’s just that the Bot is very stupid and only drops a block if it is facing an empty square next to a block that is also in front of the Bot. So it can be adjacent to a block and space but will not drop because it is looking the wrong way.
OK, then let’s do this:
class Bot:
def __init__(self, x, y, direction=Direction.EAST):
...
self.state = Walking()
def do_something(self):
self.update()
self._knowledge.tired -= 1
self.state = self.state.update(None, self._knowledge)
actions = self.state.action(self._knowledge)
for action in actions:
match action:
case 'take':
self.world.take_forward(self)
case 'drop':
self.world.drop_forward(self, self.inventory[0])
case _:
assert 0, f'no case {action}'
self.move()
Two tests break, for example:
> assert isinstance(bot.state._state, Looking)
E AttributeError: 'Looking' object has no attribute '_state'
And we need to change bot.state._state to just bot.state. Same thing in another place. Green. Commit: No longer using Machine in prod.
We do have lots of tests that are trying to use it, I think. Let’s see.
Most of them are direct tests on the method object Machine. Those are covered now by the direct tests on the state classes. I remove the whole test class. There are no references to Machine class. Remove it.
Green. Commit: Machine class all gone byebye
Article very long but one more thing …
def do_something(self):
self.update()
self._knowledge.tired -= 1
self.state = self.state.update(None, self._knowledge)
actions = self.state.action(self._knowledge)
for action in actions:
match action:
case 'take':
self.world.take_forward(self)
case 'drop':
self.world.drop_forward(self, self.inventory[0])
case _:
assert 0, f'no case {action}'
self.move()
This is a bit naff. All that stuff in the middle needs to be a method:
def do_something(self):
self.update()
self._knowledge.tired -= 1
self.state = self.state.update(None, self._knowledge)
self.do_state_actions()
self.move()
def do_state_actions(self):
for action in self.state.action(self._knowledge):
match action:
case 'take':
self.world.take_forward(self)
case 'drop':
self.world.drop_forward(self, self.inventory[0])
case _:
assert 0, f'no case {action}'
Green. Commit: extract method, tidying.
Let’s sum up.
Summary
This has been quite fine. I only regret that I wasn’t able to share it with my late-rising brothers Bryan and GeePaw.
Over a few days and a few hours, we have converted the Bot to use a class-per-state form of state machine, replacing the Method Object style that replaced the in-line version we originally did. Now the Bot’s three states, Walking, Looking, and Laden are represented by three tiny classes of those same names, each with just two methods, update and action. Oh, that reminds me: The update method no longer needs the machine parameter. Change signature, in each of them. PyCharm does the work. Green. Commit: remove unused update parameter.
As I was saying, three tiny classes, each with two methods. We removed the entire Machine class, folding its remaining actions up into Bot.
Today, we test-drove the implementation of the Walking state class, the final one we needed, and then in a series of small changes, removed the excess parameters for setting up and tearing down the machine objects. Most of the changes were to tests, although the code changed a bit, not to generate redundant None values, nor to expect them.
The tests broke repeatedly, as expected, but never broke in an unexpected way. Every test change was straightforward, mostly just “oh right we don’t do that any more”.
We made good use of PyCharm’s powers of refactoring, and used its usages search a time or two to find all the usages and change them in a batch. One of those times, it was clear that all the changes were in tests for the object we were about to remove, so we removed the tests as well.
We made 15 commits in two hours and probably missed no more than one opportunity to commit sooner. The overall cycle after the Walking class was in place was to find something to simplify, simplify it, fix any tests that it broke, repeat. There was no stress, other than one moment when I thought Bots were missing opportunities to drop things, but a quick more careful look disabused me of that concern.
I think that tomorrow we’ll do a final overview of the class per state machine, as it is worth a look on its own, but what we have seen here is that we can smoothly change out a design element, so long as it is nicely isolated. And nice isolation is the way we like our objects to be.
Overall, a fine result, attained in small steps with very little confusion. The very opposite of ragged. I feel much better now!
But for now, we are all tired here, you and I, so let’s have a break! See you next time!