One More Time FTW
I don’t like the match-case code for the small boxes, although it works. The code that I had before was more likeable but incorrect in some details. What shall I do? I like today’s result! Also: Resist fascism, and take care of one another.
I had thought to do a state machine but drawing some circles and arrows has convinced me that that would be difficult to understand and to change if change were ever necessary. Could change be necessary? Well, yes, because there is a bowling feature that we do not deal with, namely the “foul”. If your foot goes past a magic line, it is a foul and the pins you knock down do not count. If it was your first roll, you do get a second but the best you could do is a spare, since the first roll counts as zero. So, in principle, we could be asked to deal with the foul, which is shown by an F in the appropriate small box.
Since I feel that the match-case implementation is too hard to understand and change, I am tempted to go back to the old code, but the fact that it doesn’t work makes that not as good an idea as it might be.
Let’s try to do a little object that computes the three small squares given a tenth frame’s two or three rolls. The object must work incrementally, that is, returning blanks when a roll does not yet exist.
I’ll use the test file that I set up for the machine, since it is there. And I’ll copy over the extensive test that I have for the tenth frame:
def test_tenth(self):
self.check_case(None, None, None, ' ')
self.check_case(10, None, None, 'X ')
self.check_case(10, 10, None, 'XX ')
self.check_case(10, 5, None, 'X5 ')
self.check_case(10, 0, None, 'X- ')
self.check_case(6, 4, None, '6/ ')
self.check_case(3, 5, None, '35 ')
self.check_case(10, 10, 10, 'XXX')
self.check_case(10, 10, 5, 'XX5')
self.check_case(10, 6, 4, 'X6/')
self.check_case(10, 0, 10, 'X-/')
self.check_case(10, 0, 0, 'X--')
self.check_case(0, 10, 10, '-/X')
self.check_case(6, 4, 10, '6/X')
self.check_case(6, 4, 3, '6/3')
# cannot occur but work
self.check_case(6, 3, 3, '633')
self.check_case(6, 3, 3, '633')
def check_case(self, r1, r2, r3, result):
assert False
That’s enough to fail. Now let’s defer the calculation to a new object:
def check_case(self, r1, r2, r3, result):
maker = MarkMaker(r1, r2, r3)
marks = maker.marks()
assert marks == result
And a little object:
class MarkMaker:
def __init__(self, r1, r2, r3):
self.r1 = r1
self.r2 = r2
self.r3 = r3
def marks(self):
return ''
This is enough again to fail the test. I’m kind of wishing my one test were broken out into many, so I could better see my progress. It seems tedious to do, so let’s see how things go. I’ll try to make the first check pass.
No, by golly, I can break those tests out and I will. I’ll insert a standard def for each one and then just change the names. I do that in just a couple of minutes with names like this:
def test_tenth643(self):
self.check_case(6, 4, 3, '6/3')
Now I have 15 tests failing and somehow I consider that to be good.
Now how do I propose to do this? This passes the first test:
class MarkMaker:
def __init__(self, r1, r2, r3):
self.rolls = [r1, r2, r3]
self.index = 0
self.result = ''
def marks(self):
while self.index < 3:
roll = self.rolls[self.index]
if roll is None:
self.result += ' '
self.index += 1
return self.result
I enhance the code:
def marks(self):
while self.index < 3:
roll = self.rolls[self.index]
if roll is None:
self.result += ' '
elif roll == 10:
self.result += 'X'
self.index += 1
return self.result
Suddenly only 12 tests fail! Interesting! Which ones passed? NNN, XNN, and XXN. Interesting. I really think we’re going to need to do something more clever here but let’s push a bit.
- Note
- I’m thinking that we need to check prior rolls and probably to know what index we’re on. We might do some kind of pluggable behavior or just branch off to different handlers somehow.
Adding a check for zero gets me to only 10 failing:
def marks(self):
while self.index < 3:
roll = self.rolls[self.index]
if roll is None:
self.result += ' '
elif roll == 0:
self.result += '-'
elif roll == 10:
self.result += 'X'
self.index += 1
return self.result
The first failing one is XDN, namely 10, some non-ten, None. Let’s handle digits 1-9:
def marks(self):
while self.index < 3:
roll = self.rolls[self.index]
if roll is None:
self.result += ' '
elif roll == 0:
self.result += '-'
elif roll == 10:
self.result += 'X'
elif roll in range(1,10):
self.result += str(roll)
self.index += 1
return self.result
Only six failing out of the original 16, not bad at all. What’s next? 64N, spare in first two.
That requires either a change to the 1-9 case or a separate elif. Let’s do the former:
def marks(self):
previous = 666
while self.index < 3:
roll = self.rolls[self.index]
if roll is None:
self.result += ' '
elif roll == 0:
self.result += '-'
elif roll == 10:
self.result += 'X'
elif roll in range(1,10):
if self.index > 0:
if previous != 10 and previous + roll == 10:
self.result += '/'
else:
self.result += str(roll)
else:
self.result += str(roll)
previous = roll
self.index += 1
return self.result
I should have been committing these, they are harmless and I might like to back up. This has the errors down to just two!
I think I want to undo that change and do the separate case instead.
def marks(self):
previous = 666
while self.index < 3:
roll = self.rolls[self.index]
if roll is None:
self.result += ' '
elif roll == 0:
self.result += '-'
elif roll == 10:
self.result += 'X'
elif previous != 10 and previous + roll == 10:
self.result += '/'
elif roll in range(1,10):
self.result += str(roll)
previous = roll
self.index += 1
return self.result
That’s more clean and again only two failing. Can I guess what they are? Maybe but why not look?
They are X0X, which should return ‘X-/’ but returns ‘X-X’ and 0XX, which should return ‘-/X’ and is returning ‘-XX’
I’m discovering the 10 too soon in the if. Let’s move that down one and see what happens.
def marks(self):
previous = 666
while self.index < 3:
roll = self.rolls[self.index]
if roll is None:
self.result += ' '
elif roll == 0:
self.result += '-'
elif previous != 10 and previous + roll == 10:
self.result += '/'
elif roll == 10:
self.result += 'X'
elif roll in range(1,10):
self.result += str(roll)
previous = roll
self.index += 1
return self.result
All my tests pass. Commit: MarkMaker working.
We can simplify a bit, since we have no use for index as a member, and I’d like to get rid of the self in references to rolls as well.
def marks(self):
index = 0
rolls = self.rolls
result = ''
previous = 666
while index < 3:
roll = rolls[index]
if roll is None:
result += ' '
elif roll == 0:
result += '-'
elif previous != 10 and previous + roll == 10:
result += '/'
elif roll == 10:
result += 'X'
elif roll in range(1,10):
result += str(roll)
previous = roll
index += 1
return result
Commit: simplifying. We don’t use index except to get a roll. Therefore:
def marks(self):
rolls = self.rolls
result = ''
previous = 666
for _ in range(3):
roll = rolls.pop(0)
if roll is None:
result += ' '
elif roll == 0:
result += '-'
elif previous != 10 and previous + roll == 10:
result += '/'
elif roll == 10:
result += 'X'
elif roll in range(1,10):
result += str(roll)
previous = roll
return result
Commit again.
Reverse the list and pop from the other end:
class MarkMaker:
def __init__(self, r1, r2, r3):
self.rolls = [r3, r2, r1]
def marks(self):
rolls = self.rolls
result = ''
previous = 666
for _ in range(3):
roll = rolls.pop()
if roll is None:
result += ' '
elif roll == 0:
result += '-'
elif previous != 10 and previous + roll == 10:
result += '/'
elif roll == 10:
result += 'X'
elif roll in range(1,10):
result += str(roll)
previous = roll
return result
I don’t like the previous = 666 trick. Let’s use None and pay the price in the elif:
def marks(self):
rolls = self.rolls
result = ''
previous = None
for _ in range(3):
roll = rolls.pop()
if roll is None:
result += ' '
elif roll == 0:
result += '-'
elif previous is not None and previous != 10 and previous + roll == 10:
result += '/'
elif roll == 10:
result += 'X'
elif roll in range(1,10):
result += str(roll)
previous = roll
return result
That one line is quite long. Extract a method:
def marks(self):
rolls = self.rolls
result = ''
previous = None
for _ in range(3):
roll = rolls.pop()
if roll is None:
result += ' '
elif roll == 0:
result += '-'
elif self.is_spare(previous, roll):
result += '/'
elif roll == 10:
result += 'X'
elif roll in range(1,10):
result += str(roll)
previous = roll
return result
def is_spare(self, previous, roll):
return (previous is not None and
previous != 10 and
previous + roll == 10)
We could extract other methods here, such as is_absent, is_miss, is_strike, is_just_some_pins, but I don’t think I like that.
There is one kind of tricky bit here, involving the handling of a roll of 10: recall that we had to move the check for that after the check for spare. That’s because of the possibility of a roll of zero followed by a roll of ten. We would prefer that in a if cascade like this, all the conditions were independent. We could make that the case at the cost of another check for that case. Maybe like this:
def marks(self):
rolls = self.rolls
result = ''
previous = None
for _ in range(3):
roll = rolls.pop()
if roll is None:
result += ' '
elif roll == 0:
result += '-'
elif roll == 10 and previous == 0:
result += '/'
elif self.is_spare(previous, roll):
result += '/'
elif roll == 10:
result += 'X'
elif roll in range(1,10):
result += str(roll)
previous = roll
return result
Honestly, I think that makes it harder to figure out but I can see going either way. We’ll go with the shorter form.
This works, and I think I prefer it. Move the MarkMaker to its own file (I’ve been building it inside the test file) and install it:
class Frame:
def get_tenth_frame_boxes(self):
first, second, third = self.first, self.second, self.third
maker = MarkMaker(first, second, third)
result = maker.marks()
return list(result)
That replaces the very messy match-case that we had a day or so ago. Commit: use MarkMaker for tenth-frame boxes.
Let’s sum up.
Summary
Well. I am sure that I like today’s code better than the match-case solution, which you can find in Gumption Trap if you’re interested. The solution prior to that did not work quite right and looked like this:
def tenth_frame(self, n):
try:
return self._tenth_frame(n)
except Exception:
return ''
def _tenth_frame(self, n):
roll = self.rolls[(self.start + n)]
match n:
case 0:
return self.left_cell(roll)
case 1:
return self.middle_cell(roll)
case _:
return self.right_cell(roll)
def left_cell(self, roll):
return 'X' if self.strike(roll) else self.dash_for_zero(roll)
def middle_cell(self, second_roll):
first_roll = self.rolls[self.start]
return self.score_second_of_two_rolls(first_roll, second_roll)
def right_cell(self, third_roll):
second_roll = self.rolls[self.start + 1]
return self.score_second_of_two_rolls(second_roll, third_roll)
def score_second_of_two_rolls(self, first_roll, second_roll):
if self.strike(first_roll) and self.strike(second_roll):
return 'X'
elif self.spare(first_roll, second_roll):
return '/'
else:
return self.dash_for_zero(second_roll)
def strike(self, roll):
return roll == 10
def spare(self, first_roll, second_roll):
return not self.strike(first_roll) and first_roll + second_roll == 10
I think it’s fair to say that the MarkMaker is better than that. So I am pleased with the MrakMaker solution and feel that it is better than the prior versions by any reasonable standard. Unless “you only get one try at anything” is a reasonable standard, which in my strong view it is not.
That said, I do feel that for some reason, this solution has taken me longer to create than it “should” have, given that it is now pretty simple and clear. Arguably the match-case should never have been accepted, but it had the advantage over the prior code that it was correct and the prior code was not. So we sort of had to ship the match-case to fix the defect.
At the macro scale, I think that it is always worth-while to explore alternative solutions, even to solved problems, because doing so is sure to teach us things and very often results in better production code. And I freely grant that in a work situation, a developer may not feel that they “have the time” to revisit code that has been shipped, or even to refactor it much if it needs fixing or changing.
I would put the bulk of the “blame” for that situation on management, but not all of it. I think that in many situations, developers put themselves under more pressure than is necessary, and the result is worse than was reasonably attainable. So if I gave advice, which I do not, I might advise developers to build the habit of estimating with some slack in the estimate, if they must estimate, and to build the habit of working slowly and smoothly, rather than rushing.
For me, today, I like what we have today better than yesterday. I wish that were true on every dimension of your days and mine.
See you next time, and remember: resist fascism, and take care of one another.