Marks
I think today we can do some work on the ‘marks’. Also: Resist fascism, and take care of one another!
- TL;DR
-
No big discoveries today other than a bug in PyCharm. Just making all the mark tests run from the previous implementation, and two (2!) reverts when refactoring improvements augur in.
-
Fight fascism and write to your congress people to do the same.
I believe we have comprehensive mark tests over in the previous bowling exercise. Let’s see what we can find over there.
def test_normal_strings(self):
f = self.create_frame(10)
center, right = f.get_normal_boxes()
assert center == 'X'
assert right == ' '
f = self.create_frame(6, 4)
center, right = f.get_normal_boxes()
assert center == '6'
assert right == '/'
f = self.create_frame(6, 3)
center, right = f.get_normal_boxes()
assert center == '6'
assert right == '3'
f = self.create_frame()
center, right = f.get_normal_boxes()
assert center == ' '
assert right == ' '
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')
def get_tenth_frame(self, r1, r2, r3):
s = Scorekeeper()
for _frame in range(1,10):
s.roll(0)
s.roll(0)
s.roll(r1)
s.roll(r2)
s.roll(r3)
return s.frame(9)
def get_boxes(self, f):
return ''.join(
f.get_tenth_frame_boxes()
)
def check_case(self, r1, r2, r3, result):
f = self.get_tenth_frame(r1, r2, r3)
assert self.get_boxes(f) == result
I like the notation used in the tenth frame check, and since (we believe) that we just have one kind of box object now, we can use the same technique, whatever it turns out to be, for normal and tenth frames. I’ll bring over just the tests and edit them into one. I see right away that I hate that long set of checks, because I ever get to see a passing test until all of them work. I need some cheering on here, folks. So we’ll break that big test out into a bunch.
They look like this:
def test_marks(self):
self.check_case([None, None, None], ' ')
def test_a(self):
self.check_case([10, None, None], 'X ')
def test_b(self):
self.check_case([10, 10, None], 'XX ')
def check_case(self, rolls, expected):
box = Box()
for roll in rolls:
if roll is not None:
box.add_score(roll)
marks = box.marks()
assert marks == expected
I can make one pass by Fake It Till You Make It:
class Box:
def marks(self):
# return a string containing all three marks (normal + tenth)
return ' '
Now we elaborate that method to call three others:
def marks(self):
# return a string containing all three marks (normal + tenth)
return ''.join([self.first_mark(), self.second_mark(), self.third_mark()])
def marks(self):
# return a string containing all three marks (normal + tenth)
return ''.join([self.first_mark(), self.second_mark(), self.third_mark()])
def first_mark(self):
return ' '
def second_mark(self):
return ' '
def third_mark(self):
return ' '
Still quite fake, of course. I look at the second test, which is now the first one failing and I see this:
def test_a(self):
self.check_case([10, None, None], 'X ')
Interesting, in that our frame is not ready to score but we expect to get some marks out of it. I guess that’s OK. Let’s creep up on the answer:
def first_mark(self):
if self._scores[0] == 10:
return 'X'
return ' '
I expected that to work. Why didn’t it? Ask the test. The mark code will have to check to be sure rolls are there before asking for them, or else we need a tricky get_roll method. I’ll do it longhand for now.
def first_mark(self):
if len(self._scores) < 1:
return ' '
if self._scores[0] == 10:
return 'X'
return ' '
I have 14 of 16 failing now. I think I’ll elaborate the first one before moving on.
def first_mark(self):
if len(self._scores) < 1:
return ' '
score = self._scores[0]
if score == 10:
return 'X'
elif score == 0:
return '-'
else:
return str(score)
I sort of regret doing that: it didn’t fix any tests. So I get no spark of joy out of it. I’ll leave it. Let’s find an easy test to fix. Next in the list is two tens in a row.
def second_mark(self):
if len(self._scores) < 2:
return ' '
score = self._scores[1]
if score == 10:
return 'X'
elif score == 0:
return '-'
else:
return str(score)
Only ten now failing, and we’re creating a lot of duplication. Duplication always makes me feel certain that the ode will get much nicer in the near future. So I am feeling that spark of joy now. What’s failing now? A spare:
self.check_case([6, 4, None], '6/ ')
Great: That’s in second_mark which we just took a cut at. We’ll do this:
def first_mark(self):
if len(self._scores) < 1:
return ' '
score = self._scores[0]
return self.possible_X(score)
def possible_X(self, score):
if score == 10:
return 'X'
elif score == 0:
return '-'
else:
return str(score)
def second_mark(self):
if len(self._scores) < 2:
return ' '
score = self._scores[1]
return self.possible_X(score)
I reduced that duplication right away, more to give myself room to work. We’ll still find improvements, I’m sure.
def second_mark(self):
if len(self._scores) < 2:
return ' '
second_roll = self._scores[1]
first_roll = self._scores[0]
if first_roll == 10:
return self.possible_X(second_roll)
elif first_roll + second_roll == 10:
return '/'
else:
return self.possible_X(second_roll)
Only nine failing now.
Next one is three strikes in a row, so we move to our third mark/
def third_mark(self):
if len(self._scores) < 3:
return ' '
return self.possible_X(self._scores[2])
That gets us down to three failures. I suspect they will have to do with spares in the second and third positions. Here’s the first one.
Expected :'X6/'
Actual :'X64'
We’ll copy the second mark’s logic over.
def third_mark(self):
if len(self._scores) < 3:
return ' '
second_roll = self._scores[1]
third_roll = self._scores[2]
if second_roll == 10:
return self.possible_X(self._scores[2])
elif second_roll + third_roll == 10:
return '/'
else:
return self.possible_X(third_roll)
Frustratingly, that leaves one test failing: I was hoping to wrap up all three. What is it?
def test_o(self):
# cannot occur but work
self.check_case([6, 3, 3], '633')
And:
Expected :'633'
Actual :'63 '
A quick print tells me that I didn’t get to the case, and I know why: our Box never stores the third roll if it doesn’t need it. Only the tenth frame unconditionally gets that third roll.
Well, I was still somewhat open to the possibility that the tenth frame had to be special, but I had hoped that it would not.
I immediately begin to think of some easy hacks, such as telling the frame that it is the tenth, but before I start slashing, let’s think about scoring that frame.
Our normal frame only scores three rolls if it is a mark: a strike or spare. Oh, wait! Look at that test! It cannot occur! The player will not get a third roll unless he has at least a spare in the first two. I can delete that test! Callooh! Callay!
We are green. Commit: all tests for marks passing but one ignored normal frame marks tests.
Not the most graceful commit comment I’ve ever made. Anyway let’s look at the ignored test now and see if we want to do some more checking:
@pytest.mark.skip("not ready")
def test_normal_strings(self):
f = self.create_frame(10)
center, right = f.get_normal_boxes()
assert center == 'X'
assert right == ' '
f = self.create_frame(6, 4)
center, right = f.get_normal_boxes()
assert center == '6'
assert right == '/'
f = self.create_frame(6, 3)
center, right = f.get_normal_boxes()
assert center == '6'
assert right == '3'
f = self.create_frame()
center, right = f.get_normal_boxes()
assert center == ' '
assert right == ' '
I believe that all those cases are included in the tests that are passing:
def test_marks(self):
self.check_case([None, None, None], ' ')
def test_a(self):
self.check_case([10, None, None], 'X ')
def test_e(self):
self.check_case([6, 4, None], '6/ ')
def test_f(self):
self.check_case([6, 3, None], '63 ')
Do you agree? Then we can remove that test entirely as redundant, extraneous, unnecessary, and not needed.
44 tests, all green. Commit: marks are tested and working.
I’d like to see those tests given better names than test_b, which was the best I came up with when my mission was just to break them all apart. More important, however, is to clean up the mark creation code, so we’ll do that first:
def first_mark(self):
if len(self._scores) < 1:
return ' '
score = self._scores[0]
return self.possible_X(score)
def possible_X(self, score):
if score == 10:
return 'X'
elif score == 0:
return '-'
else:
return str(score)
def second_mark(self):
if len(self._scores) < 2:
return ' '
second_roll = self._scores[1]
first_roll = self._scores[0]
if first_roll == 10:
return self.possible_X(second_roll)
elif first_roll + second_roll == 10:
return '/'
else:
return self.possible_X(second_roll)
def third_mark(self):
if len(self._scores) < 3:
return ' '
second_roll = self._scores[1]
third_roll = self._scores[2]
if second_roll == 10:
return self.possible_X(self._scores[2])
elif second_roll + third_roll == 10:
return '/'
else:
return self.possible_X(third_roll)
Let’s generalize possible_X so that it can return either X or / for a ten score otherwise the value. We have to have two input values now, the one to mark and the one to check. Like this:
def possible_mark(self, mark, score, possible_10):
if possible_10 == 10:
return mark
elif score == 0:
return '-'
else:
return str(score)
And we use it like this:
Revert!
Meh! Did something wrong. Got down to all but one test working. Not good enough. Revert. I tried that by changing the method and then applying it and something didn’t work. Let’s see if we can do something a bit more automated.
second_mark and third_mark include some duplication:
def second_mark(self):
...
if first_roll + second_roll == 10:
return '/'
else:
return self.possible_X(second_roll)
def third_mark(self):
...
if second_roll + third_roll == 10:
return '/'
else:
return self.possible_X(third_roll)
Those initial if statements used to be elif but there is a return in the prior code so they can be if. That makes them more amenable to refactoring.
def second_mark(self):
if len(self._scores) < 2:
return ' '
second_roll = self._scores[1]
first_roll = self._scores[0]
if first_roll == 10:
return self.possible_X(second_roll)
return self.possible_slash(first_roll, second_roll)
def third_mark(self):
if len(self._scores) < 3:
return ' '
second_roll = self._scores[1]
third_roll = self._scores[2]
if second_roll == 10:
return self.possible_X(self._scores[2])
return self.possible_slash(second_roll, third_roll)
def possible_slash(self, first_roll, second_roll):
if first_roll + second_roll == 10:
return '/'
else:
return self.possible_X(second_roll)
This is green. Commit: refactoring.
Now both those methods are calling possible_X but in fact X is not possible: we know this is not a mark. So let’s have a new method dash_for_zero …
Again, the next step results in tests failing. There is something going on here that I do not understand entirely, but it has to do with the fact that some of the calls to possible_X are actually returning an X when, in my mind, I do not expect one.
We have a nice safe point, let’s revert again and try another angle.
Revert!
I’m going to inline all my little functions and start anew.
Yikes! PyCharm horks the inlining, really mashing the code badly. I’ll submit a bug report on that. First, let me clear out its caches and try again. Still horks it. Revert.
I can inline possible_slash, getting this:
def second_mark(self):
if len(self._scores) < 2:
return ' '
second_roll = self._scores[1]
first_roll = self._scores[0]
if first_roll == 10:
return self.possible_X(second_roll)
if first_roll + second_roll == 10:
result = '/'
else:
result = self.possible_X(second_roll)
return result
def third_mark(self):
if len(self._scores) < 3:
return ' '
second_roll = self._scores[1]
third_roll = self._scores[2]
if second_roll == 10:
return self.possible_X(self._scores[2])
if second_roll + third_roll == 10:
result = '/'
else:
result = self.possible_X(third_roll)
return result
I’ll make those more alike, including putting back the elif, and setting result in the first if:
def second_mark(self):
if len(self._scores) < 2:
return ' '
prior_roll = self._scores[0]
current_roll = self._scores[1]
if prior_roll == 10:
result = self.possible_X(current_roll)
elif prior_roll + current_roll == 10:
result = '/'
else:
result = self.possible_X(current_roll)
return result
def third_mark(self):
if len(self._scores) < 3:
return ' '
prior_roll = self._scores[1]
current_roll = self._scores[2]
if prior_roll == 10:
result = self.possible_X(current_roll)
elif prior_roll + current_roll == 10:
result = '/'
else:
result = self.possible_X(current_roll)
return result
That should be about enough duplication to convince anyone that we should do this:
def second_mark(self):
if len(self._scores) < 2:
return ' '
prior_roll = self._scores[0]
current_roll = self._scores[1]
return self.possible_slash_or_X(prior_roll, current_roll)
def third_mark(self):
if len(self._scores) < 3:
return ' '
prior_roll = self._scores[1]
current_roll = self._scores[2]
return self.possible_slash_or_X(prior_roll, current_roll)
def possible_slash_or_X(self, prior_roll, current_roll):
if prior_roll == 10:
result = self.possible_X(current_roll)
elif prior_roll + current_roll == 10:
result = '/'
else:
result = self.possible_X(current_roll)
return result
Green. Commit: refactoring.
It’s time to stop. The code is somewhat improved, most of the duplication taken out. And it’s working, which is always a high priority. Let’s sum up.
Summary
Making the marks work was pretty straightforward. It was helpful to break that big test down into separate tests, and I wish I had taken the time to give them better names. I might do that in my copious free time, but no sense bothering you with it unless I come up with something really marvelous.
I am quite pleased that the marks were all able to be handled in the existing Box class. The only case in the tests that isn’t just like the prior program was a tenth-frame case that cannot happen, 6, 3, 3. You can’t roll that last 3 because you have neither a strike nor a spare in the first two rolls so, sorry, bunkie, no third roll for you.
The mark code is fairly well factored, I think, but the meaning does not jump out at you. It seems to me, intuitively, that there is some kind of simple pattern to the marks, and that we just haven’t found it yet. And I was thinking that we might find at least some simplification by separating out the two-mark case from the three-mark final frame case … but wouldn’t the first two marks be exactly the same anyway?
I’d like to find something better.
Anyway, now I have a bug report to create for JetBrains. See you next time, and until then: fight fascism and write to your congress people to do the same.