3
\$\begingroup\$

I've been writing basic Python scripts for a while now to help process data or automate some task but I've decided I should start picking up unit testing and objective orientated programming (the vast majority of my scripts so far have been procedural).

As a starter I decided to follow along with Uncle Bob's bowling scoring kata to try and get my mind around TDD and the idea of writing the absolute minimal code at every step to either make the test go red or green (plus any refactoring steps).

As it's a bare bones example of following TDD the main program doesn't actually have an entry point other than via the tests.

Things that stand out to my beginner's eye:

  • There are a lot of selfs which look like a lot of visual clutter when I read through the code. Is there a better way of doing this? I think it's the density of them that really gets me so I wasn't sure if I could abstract some of them somehow?

  • unittest seems to have a lot of boilerplate. I had a play with nose a while back which seemed to strip a lot of that out but I thought it might be a good idea to start properly with unittest until I have a use case for anything that nose (or any other library) offers.

bowling_game.py

#!/usr/bin/env python

class Game:

  _rolls = [0] * 21
  _current_roll = 0

  def roll(self, pins):
    self._rolls[self._current_roll] = pins
    self._current_roll += 1

  def score(self):
    score = 0
    frame_index = 0
    for frame in range(0, 10):
      if self._is_strike(frame_index):
        score += 10 + self._strike_bonus(frame_index)
        frame_index += 1
      elif self._is_spare(frame_index):
        score += 10 + self._spare_bonus(frame_index)
        frame_index += 2
      else:
        score += self._rolls[frame_index] + self._rolls[frame_index + 1]
        frame_index += 2
    return score

  def _sum_of_balls_in_frame(self, frame_index):
    return self._rolls[frame_index] + self._rolls[frame_index + 1]

  def _spare_bonus(self, frame_index):
    return self._rolls[frame_index + 2]

  def _strike_bonus(self, frame_index):
    return self._rolls[frame_index + 1] + self._rolls[frame_index + 2]

  def _is_spare(self, frame_index):
    return self._rolls[frame_index] + self._rolls[frame_index + 1] == 10

  def _is_strike(self, frame_index):
    return self._rolls[frame_index] == 10

bowling_game_test.py

#!/usr/bin/env python

import unittest

from bowling_game import Game

class BowlingGameTest(unittest.TestCase):

  def setUp(self):
    self.g = Game()

  def roll_many(self, rolls, pins):
    for roll in range(0, rolls):
      self.g.roll(pins)

  def roll_spare(self):
    self.g.roll(5)
    self.g.roll(5)

  def roll_strike(self):
    self.g.roll(10)

  def test_gutter_game(self):
    rolls = 20
    pins = 0
    self.roll_many(rolls, pins)
    self.assertEquals(self.g.score(),0)

  def test_all_ones(self):
    rolls = 20
    pins = 1
    self.roll_many(rolls, pins)
    self.assertEquals(self.g.score(),20)

  def test_one_spare(self):
    self.roll_spare()
    self.g.roll(3)
    self.roll_many(17, 0)
    self.assertEquals(self.g.score(),16)

  def test_one_strike(self):
    self.roll_strike()
    self.g.roll(3)
    self.g.roll(4)
    self.roll_many(16, 0)
    self.assertEquals(self.g.score(),24)

  def test_perfect_game(self):
    self.roll_many(12, 10)
    self.assertEquals(self.g.score(),300)

if __name__ == '__main__':
    unittest.main()

The commit history is available on GitHub if anyone fancies taking a look at that to see where I might have gone better with the red -> green -> refactor cycle.

\$\endgroup\$
1
\$\begingroup\$

Details

With Python 3.2, I got the following warning :

file.py:72: DeprecationWarning: Please use assertEqual instead.
self.assertEquals(self.g.score(),24)

which is easily fixed.

Class variables

Class variables are a pretty messy topic in Python and it is easy to get it wrong. The solution is quite simple : avoid class variables unless you know what you are doing and move this :

_rolls = [0] * 21 _current_roll = 0

to a proper __init__ method.

You can see the issue by adding the following check (it uses a method I'll define later):

def test_toto(self):
    self.roll([10])
    self.assertEqual(self.g.score(), 10)

Style

Your code looks good. However, your indentation and your spacing could be improved to comply to PEP 8.

Suggestion

Instead of having roll_many roll the same score every time. It could be an idea to have more flexibility. I've implemented such a thing with an array of scores :

class BowlingGameTest(unittest.TestCase):

    def setUp(self):
        self.g = Game()

    def roll(self, rolls):
        for val in rolls:
            self.g.roll(val)

    def test_gutter_game(self):
        rolls = 20
        pins = 0
        self.roll([pins] * rolls)
        self.assertEqual(self.g.score(), 0)

    def test_all_ones(self):
        rolls = 20
        pins = 1
        self.roll([pins] * rolls)
        self.assertEqual(self.g.score(), 20)

    def test_one_spare(self):
        self.roll([5, 5, 3])
        self.assertEqual(self.g.score(), 16)

    def test_one_strike(self):
        self.roll([10, 3, 4] + [0] * 16)
        self.assertEqual(self.g.score(), 24)

    def test_perfect_game(self):
        self.roll([10] * 12)
        self.assertEqual(self.g.score(), 300)

Points to improve/think about

Having 21 hardcoded in your __init__ leads to 2 issue :

  • we have a magic number

  • one may think that the number of rolls will be handled by the class (which is not entirely the case)

Maybe your function could be a simple function returning a score for a list of throws (code will maybe arrive later).

\$\endgroup\$
  • \$\begingroup\$ After a quick bit of reading around class variables I'm now not quite sure why you'd want them outside the __init__ but I do see it semi often in other Python code that I read. Probably one for some future reading. The PEP8 points are an obvious one and something I should have done before posting really but thanks for pointing it out. \$\endgroup\$ – ydaetskcoR Mar 28 '16 at 7:50
  • \$\begingroup\$ I was half considering moving the 21 into a constant such as MAX_ROLLS_IN_GAME because the class (currently) is meant to cover a whole game. There could be an argument that if this was part of a whole, playable bowling game then this could just be a scoring class and then I'd probably want to turn the class into one that simply returns a score for a list of throws (potentially saying when a frame is "locked" too) so it could be calculating the score as the game goes on. \$\endgroup\$ – ydaetskcoR Mar 28 '16 at 7:52
1
\$\begingroup\$

It doesn't look like too many selfs to me.


class Game:

  _rolls = [0] * 21
  _current_roll = 0

  ...

3 things right off the bat:

  1. What's with the blank line between class Game: and _rolls = [0] * 21?
  2. You are using 2 spaces to indent. Use 4 spaces.
  3. Shouldn't this be in the __init__ function?:

    _rolls = [0] * 21
    _current_roll = 0
    

So it would look like:

class Game:
    def __init__(self):
        self._rolls = [0] * 21
        self._current_roll = 0

    def roll(self, pins):
        self._rolls[self._current_roll] = pins
        self._current_roll += 1

    ...
\$\endgroup\$
  • \$\begingroup\$ Yeah I should have picked up on the PEP8 stuff myself really and made those changes before posting. Thanks for the pointer though. And as mentioned in my comment on Josay's answer I probably need to do some more reading around class variables. \$\endgroup\$ – ydaetskcoR Mar 28 '16 at 7:53
1
\$\begingroup\$

You can improve the re-usability / generality / dubug-ability of the code by basing score on a helper that returns all the scores:

  def scores(self):
    frame_index = 0
    for _ in range(0, 10):
      if self._is_strike(frame_index):
        yield 10 + self._strike_bonus(frame_index)
        frame_index += 1
      elif self._is_spare(frame_index):
        yield 10 + self._spare_bonus(frame_index)
        frame_index += 2
      else:
        yield self._rolls[frame_index] + self._rolls[frame_index + 1]
        frame_index += 2


  def score(self):
    return sum(self.scores())

As a nitpick, frame is not used, so you should use _ [UNDERSCORE] in place of it by convention.

\$\endgroup\$
  • \$\begingroup\$ Can you provide any examples about how that form makes it easier to test or re-use? I've only used generators a few times previously and I'm not sure I see the purpose here. I'm also not sure I agree with you on replacing frame with _. Even though frame isn't used it helps explain why there is a 10 iteration loop in the function and improves readability for me. I'm sure there's probably an even better way of making that more readable too but going for an underscore here instead just makes it seem slightly confusing at first glance. \$\endgroup\$ – ydaetskcoR Mar 28 '16 at 8:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.