One Last(?) Look
Let’s take a quick look at our bowling scoring state machine. Just because. We find some nice improvements in this ‘done’ program. Also: Resist fascism, and take care of one another!
Our state machine bowling solution really only has two classes. One of them is this one:
class ZeroBox:
def total_score(self):
return 0
The job of that class is to serve as the left-hand end of a chain of ten Boxes that represent the ten frames of the bowling game. It’s just there to allow the total score to start off being zero and accumulate thereafter. We might mention that again below.
The work of scoring is done by this class’s big brother, named Box.
Before we even look at it, we have something worth thinking about. Is “Box” a good name for this thing? Is it a box? Do cats sit in it? I don’t think so! What is it really? It is an object that scores a single frame of bowling, using a state machine design. Shouldn’t it be named something like Frame, or FrameScorer, or FrameScoringMachine?
I think its implementation details are its own concern, so maybe it shouldn’t be called “machine” or “state machine”. the word “scorer” is awkward. I think it is an active object, so calling it just a “frame score” seems not quite right.
Naming is hard, but I think we can be sure that even in a program the size of a real bowling scoring system, the name “Box” is probably not a good one for a class of this importance. It holds all the information about a single frame, including its current cumulative score and the values of the small mark boxes. Maybe it’s a “frame information”. Maybe we forget worrying about the fact that it is is a very active little box and call it a “frame score”.
Of the ideas so far, I think I like FrameScore best, but I don’t love it. I don’t love Box either, mind you.
We’ll set that aside and consider the actual code, all 85 lines of it.
Init
We look at the init first. The init often offers clues.
class Box:
def __init__(self, previous_box = None):
self.previous_box = previous_box if previous_box else ZeroBox()
self._scores = []
self._next_action = self._record_first_roll
self._mark_1 = ' '
self._mark_2 = ' '
self._mark_3 = ' '
The three mark things, with numbers on them, are a bit odd. One could imagine they’d be represented by an array. We’ll keep an eye out for them.
The list _scores. Are those really scores? No. That is a list of the pins dropped in the rolls that the frame considers when scoring. They are the pin drop counts applicable to this frame. Would _drops be better? _applicable_pin_drops? We can rename with a few keystrokes, so we can freely decide.
Naming is hard. We’ll let this name ride as well, but as we read the code let’s see whether we find the name to be OK or whether there’s a better one.
Oh! Isn’t previous_box private? Make it so.
class Box:
def __init__(self, previous_box = None):
self._previous_box = previous_box if previous_box else ZeroBox()
...
Public Methods
Now let’s look at the public methods of this object. It has very few.
def add_score(self, pins):
return self._next_action(pins)
def score(self):
if self._next_action == self._frame_complete:
return sum(self._scores)
else:
return None
def total_score(self):
prev = self._previous_box.total_score()
mine = self.score()
if prev is None or mine is None:
return None
return prev + mine
def marks(self):
return ''.join([self._mark_1, self._mark_2, self._mark_3])
There’s the word score, and it sure is bearing a lot of weight. Are we adding a score? We’re recording the results of one roll of one player’s game. Let’s try renaming that to add_dropped_pins and see how we like it.
The methods score and total_score. The first is the score for this frame, and the second is the cumulative score. Here again some renaming might be called for. Of course, if we have lots of users of our code, we have to be careful about renaming public methods. But while that’s certainly true, it doesn’t lessen the importance of good names.
Hey, wait! Why is score public? It has a lot of tests using it, but there is no legitimate use of it outside our own tests and this class. Make it private.
Both of those renames were problematical, in that there are still two other classes in this repo, StrikeBox and SpareBox and they needed renaming as well. Our tests, you may recall, test both those old classes and the new Box object, which supplants the other two.
We should remove those other classes, as we know they will not be used. I do so. It’s a bit tedious, had to remove some test files and a couple of specific tests. And, of course, now our parameterized tests are a bit odd, since they test a list of just one class. No worried, but it’s the sort of thing one might want to clean up.
Reflection
Think about this! We’ve looked at, what, three methods of this perfectly good class and found at least a half-dozen things that really could benefit from improvement. We’ve committed four times, some simple renames and at least one commit for removing two classes we don’t need. (Those could have been done before — but then everything could have been done before — it just wasn’t.)
And while some of these changes are arguably not that important, it all adds to the cognitive load, doesn’t it? So when we have the chance, if the changes are easy, I think it’s worth it to do them, because reducing our cognitive load helps us.
What would have been better? Well, if in fact we thought we wanted our new Box and the old SpareBox and StrikeBox to have the same protocol, we could have build an abstract superclass for all three, and then PyCharm would have been much more capable of the renaming that we did.
In your work life, maybe you don’t have time to do reviews like this. The more we discover this morning, the more obvious it becomes that having that time would be valuable, because the changes we’re making makes things more clear, and generally smaller, and that means our work will go more smoothly in the future.
Something to think about.
Moving Right Along
OK, where were we? Oh, yes, right here, at the second and third public methods of our Box:
def total_score(self):
prev = self._previous_box.total_score()
mine = self._score()
if prev is None or mine is None:
return None
return prev + mine
def marks(self):
return ''.join([self._mark_1, self._mark_2, self._mark_3])
What’s going on here? Well, this is the accumulated score (not the total) for the frame, and it can be None. What does that mean? Here’s a line of code that might help:
def total_score(self):
not_yet_available = None
prev = self._previous_box.total_score()
mine = self._score()
if prev is not_yet_available or mine is not_yet_available:
return not_yet_available
return prev + mine
Would you prefer an else: there? I think I might.
def total_score(self):
not_yet_available = None
prev = self._previous_box.total_score()
mine = self._score()
if prev is not_yet_available or mine is not_yet_available:
return not_yet_available
else:
return prev + mine
Commit: minor refactoring for clarity.
We’re left with marks. We’ll continue to let that ride.
Now we’re down to the private methods, the internals of how this thing works. We saw in the add_dropped_pins that we do this:
def add_dropped_pins(self, pins):
return self._next_action(pins)
And we recall from the init that _next_action is initialized to _record_first_roll, and sure enough that’s the next thing in the file:
def _record_first_roll(self, pins):
self._scores.append(pins)
self._mark_1 = self._mark(pins)
if pins == 10:
self._next_action = self._record_strike_bonus_1
else:
self._next_action = self._record_second_roll
return True
I think that if I were just to wander in here from somewhere and see that code, I’d wonder what was going on. After a while, I’m sure I’d figure out that what we have here is a finite state machine implemented by poking methods into _next_action.
Would it be more obvious if _next_action were renamed to _state or _machine_state or something like that? Or should we have some documentation comment or something to explain what’s going on? Personally, I do not favor that kind of documentation, as in my experience it is rarely up to date and often more confusing than good code would be. But that does put a burden on good naming.
Now there is another formulation of a state machine that might make this all more clear. What if we could write this:
def add_score(pins):
result, new_state = self._state(pins)
self._state -= new_state
return result
That would make it more clear what’s going on, I think. Should we refactor to do it that way?
Interesting question. I’m going to put that code in and see what breaks and how hard it is to fix. When it gets too gnarly, I’ll just revert. This is an experiment, but if it goes smoothly, we’ll keep it.
Of course 29 tests break. I think we only have 29 tests. Here’s the first method that needs fixing:
def _record_first_roll(self, pins):
self._scores.append(pins)
self._mark_1 = self._mark(pins)
if pins == 10:
self._next_action = self._record_strike_bonus_1
else:
self._next_action = self._record_second_roll
return True
I think it needs to go like this:
def _record_first_roll(self, pins):
self._scores.append(pins)
self._mark_1 = self._mark(pins)
if pins == 10:
next_action = self._record_strike_bonus_1
else:
next_action = self._record_second_roll
return True, next_action
Or, better yet, like this:
def _record_first_roll(self, pins):
self._scores.append(pins)
self._mark_1 = self._mark(pins)
if pins == 10:
return True, self._record_strike_bonus_1
else:
return True, self._record_second_roll
I’ll just push on with these changes, unless something interesting arises. Nothing does. It goes swimmingly, with methods transforming as above or even more simply.
Commit: state methods now return Result and next state.
What might be really good would be to type hint those methods.
def _record_first_roll(self, pins) -> tuple[bool, Callable]:
self._scores.append(pins)
self._mark_1 = self._mark(pins)
if pins == 10:
return True, self._record_strike_bonus_1
else:
return True, self._record_second_roll
And let’s do this:
class Box:
def __init__(self, previous_box = None):
self._previous_box = previous_box if previous_box else ZeroBox()
self._scores = []
self._state: Callable[[int], tuple[bool, Callable]] = self._record_first_roll
self._mark_1 = ' '
self._mark_2 = ' '
self._mark_3 = ' '
I think that’s an improvement and might even be legit. Commit: type hint state machine methods.
In for a penny, I do a few more:
class Box:
def __init__(self, previous_box = None):
self._previous_box: ZeroBox|Box = previous_box if previous_box else ZeroBox()
self._scores:list[int] = []
self._state: Callable[[int], tuple[bool, Callable]] = self._record_first_roll
self._mark_1 = ' '
self._mark_2 = ' '
self._mark_3 = ' '
And we can, of course, hint the state methods, and all of them really. This can be done all at once or over a period of time. Can’t hurt, might help.
I think we’ve made our point. Let’s sum up.
Summary
In this “done” program, we’ve identified a number of ways that it could use a little improvement:
- Our ability to understand and maintain the code would benefit from making the names, both public and private, more clear.
- A better way of expressing the state machine’s behavior made it more understandable and likely more able to be changed if need be. (Granted, in bowling, this is unlikely. But let’s pretend this mattered.)
- Type hints are easy to put in and enable PyCharm to provide better warnings and suggestions.
I’ve done seven commits over a couple of hours, and could have done ten or more, had I been attentive to every time I had a green bar. Only the conversion of the state machine return took any time at all, and that was less than ten minutes.
When are things like this “worth doing”? If I were working for a living, then I’d try to do a quick review at the end of every day’s session, and at the beginning of the next. I would not look all around for code that just needed refactoring. Instead, I’d look at the code that we had other reasons to work on, whether adding new capabilities, making required adjustments, or, yes it happens, fixing defects. It’s probably best to look carefully at code that turns out to have defects: one frequent cause of defects is that the code is not clear or well written.
Naturally, all this goes better with good tests, but everything does, so I’d be working right along to keep testing just one character ahead of the code.
What should you do? Ask yourself: I’m not the boss of you. You get to decide how to work so that it best suits you.
But please do consider writing to your congress people and please fight fascism and authoritarianism in any and every way that you can.
See you next time!
P.S.
replace this:
def add_dropped_pins(self, pins: int):
result, new_state = self._state(pins)
self._state = new_state
return result
With:
def add_dropped_pins(self, pins: int):
result, self._state = self._state(pins)
return result