A famous author also refreshing his Smalltalk proposes a single method solution. Going where no man dares to tread, I'm going to try to refactor it into the style I consider to be "good". You decide.

Refactoring Another Version

The famous author in question proposed this single-method solution:

score: rolls

  | score frameStart |
  score := 0.
  frameStart := 1. 

  10 timesRepeat: [  | roll1 roll2  open spare strike |
    roll1 := rolls at: frameStart.  
    roll2 := rolls at: frameStart + 1.
    open := roll1 + roll2 < 10.
    spare := (roll1 < 10)   and:  (roll1 + roll2 = 10).
    strike := roll1 = 10.

    score := score + roll1 + roll2.
    ( spare or: strike )  
      ifTrue: [ score := score + (rolls at: frameStart + 2)].

    ( open or: spare ) ifTrue:  [ frameStart := frameStart + 2 ].
    ( strike ) ifTrue:  [ frameStart := frameStart + 1 ].
  ].
  ^score

My plan is to refactor it in line with my style of Smalltalking, to see what happens. First thing is to hook it up with the tests. Alis, er, the author tested his version by sending in an array of rolls to the score method. My tests do the rolls incrementally, so I’ll add a rolls variable to the class, a #roll: method, and remove the parameter from the score method. That should let me run my tests.

 Smalltalk defineClass: #BowlingGame2
  superclass: #{Core.Object}
  indexedType: #none
  private: false
  instanceVariableNames: 'rolls '
  classInstanceVariableNames: ''
  imports: ''
  category: 'RonBowling'

initialize
  rolls := OrderedCollection new.
  ^self

roll: anInteger
  rolls add: anInteger

score

  | score frameStart |
  score := 0.
  frameStart := 1. 

  10 timesRepeat: [  | roll1 roll2  open spare strike |
    roll1 := rolls at: frameStart.  
    roll2 := rolls at: frameStart + 1.
    open := roll1 + roll2 < 10.
    spare := (roll1 < 10)   and:  (roll1 + roll2 = 10).
    strike := roll1 = 10.

    score := score + roll1 + roll2.
    ( spare or: strike )  
      ifTrue: [ score := score + (rolls at: frameStart + 2)].

    ( open or: spare ) ifTrue:  [ frameStart := frameStart + 2 ].
    ( strike ) ifTrue:  [ frameStart := frameStart + 1 ].
  ].
  ^score

My tests now run correctly. However, the compiler objects to several of the statements. It thinks, for example, that the assignment to spare:

spare := (roll1 < 10)   and:  (roll1 + roll2 = 10).

should be:

spare := (roll1 < 10)   and:  [roll1 + roll2 = 10].

Furthermore, I think it should be:

spare := roll1 < 10 and:  [roll1 + roll2 = 10].

The parens aren’t necessary, and I know Smalltalk gurus who will dis you for using unnecessary parens. Changing those, I get:

score

  | score frameStart |
  score := 0.
  frameStart := 1. 

  10 timesRepeat: [  | roll1 roll2  open spare strike |
    roll1 := rolls at: frameStart.  
    roll2 := rolls at: frameStart + 1.
    open := roll1 + roll2 < 10.
    spare := roll1 < 10  and:  [roll1 + roll2 = 10].
    strike := roll1 = 10.

    score := score + roll1 + roll2.
    ( spare or: [strike] )  ifTrue: [ score := score + (rolls at: frameStart + 2)].

    ( open or: [spare] ) ifTrue:  [ frameStart := frameStart + 2 ].
    strike ifTrue:  [ frameStart := frameStart + 1 ].
  ].
  ^score

And the tests still run.

The frameStart calculation lines are independent, but the statements as written obscure this. Also, checking open or spare is redundant if we test strike. I’d perhaps write:

score

  | score frameStart |
  score := 0.
  frameStart := 1. 

  10 timesRepeat: [  | roll1 roll2  open spare strike |
    roll1 := rolls at: frameStart.  
    roll2 := rolls at: frameStart + 1.
    open := roll1 + roll2 < 10.
    spare := roll1 < 10  and:  [roll1 + roll2 = 10].
    strike := roll1 = 10.

    score := score + roll1 + roll2.
    ( spare or: [strike] )  ifTrue: [ score := score + (rolls at: frameStart + 2)].

    frameStart := frameStart + ( strike 
      ifTrue: [ 1 ]
      ifFalse: [ 2 ] ).
  ].
  ^score

Now the compiler tells me that open’s value is never used. I’ll remove that.

score

  | score frameStart |
  score := 0.
  frameStart := 1. 

  10 timesRepeat: [  | roll1 roll2  spare strike |
    roll1 := rolls at: frameStart.  
    roll2 := rolls at: frameStart + 1.
    spare := roll1 < 10  and:  [roll1 + roll2 = 10].
    strike := roll1 = 10.

    score := score + roll1 + roll2.
    ( spare or: [strike] )  ifTrue: [ score := score + (rolls at: frameStart + 2)].

    frameStart := frameStart + ( strike 
      ifTrue: [ 1 ]
      ifFalse: [ 2 ] ).
  ].
  ^score

Tests still green. I’m inclined to extract three roll methods:

roll1
  ^rolls at: frameStart

roll2
  ^rolls at: frameStart + 1

roll3
  ^rolls at: frameStart + 2

To do that, I have to add frameStart as a member variable. Then I’ll use those methods in score:

score

  | score |
  score := 0.
  frameStart := 1. 

  10 timesRepeat: [  |  spare strike |
    spare := self roll1 < 10  and:  [ self roll1 + self roll2 = 10].
    strike := self roll1 = 10.

    score := score + self roll1 + self roll2.
    ( spare or: [strike] )  ifTrue: [ score := score + self roll3 ].

    frameStart := frameStart + ( strike 
      ifTrue: [ 1 ]
      ifFalse: [ 2 ] ).
  ].
  ^score

Tests still green. Now there’s something odd about this strike, spare stuff. (And I’m going to want open back.) We need to know strike so we know whether to increment frameStart by 1 or 2. With strike or spare, we read three rolls, otherwise just two. Let’s rewrite that. First, I’ll extract strike and spare as methods:

strike
  ^self roll1 = 10

spare
  ^self roll1 < 10  and:  [ self roll1 + self roll2 = 10].

and use them:

score

  | score |
  score := 0.
  frameStart := 1. 

  10 timesRepeat: [
    score := score + self roll1 + self roll2.
    ( self spare or: [ self strike ] )  ifTrue: [ score := score + self roll3 ].

    frameStart := frameStart + ( self strike 
      ifTrue: [ 1 ]
      ifFalse: [ 2 ] ).
  ].
  ^score

Tests still OK. Now the two assignments to score bother me, or else its the or. Let’s assume the latter and extract:

bonusFrame
  ^self spare or: [self strike]

score
    | score |
    score := 0.
    frameStart := 1.
    10 timesRepeat: 
        [score := score + self roll1 + self roll2.
        self bonusFrame ifTrue: [score := score + self roll3].
        frameStart := frameStart + (self strike ifTrue: [1] ifFalse: [2])].
    ^score

Hmm. The formatting doesn’t match mine. The method was rewritten and formatted by the refactoring browser. I kind of like it that way.

Tests are still green. I like the method this way. Some people may not like the embedded conditional, in which case they might say this:

score
    | score |
    score := 0.
    frameStart := 1.
    10 timesRepeat: 
        [score := score + self roll1 + self roll2.
        self bonusFrame ifTrue: [score := score + self roll3].
        self strike
            ifTrue: [ frameStart := frameStart + 1]
            ifFalse: [ frameStart := frameStart + 2]].
    ^score

Either way, I feel that this is more what Smalltalk is “supposed” to look like. That’s what was beaten into my head by some real experts. Even then, I may have it wrong.

Are we there yet?

Well, no. Our code has a couple of logical things done inside the loop. It updates the score, and it updates the frame pointer. Therefore those “should” be extracted as explaining methods:

score
    | score |
    score := 0.
    frameStart := 1.
    10 timesRepeat: 
        [score := score + self roll1 + self roll2.
        self bonusFrame ifTrue: [score := score + self roll3].
        self updateFrameStart].
    ^score

updateFrameStart
    self strike
        ifTrue: [ frameStart := frameStart + 1]
        ifFalse: [ frameStart := frameStart + 2]

And then …

score
    | score |
    score := 0.
    frameStart := 1.
    10 timesRepeat: 
        [score := score + self frameScore.
        self updateFrameStart].
    ^score

frameScore
    | frameScore |
    frameScore := self roll1 + self roll2.
    self bonusFrame ifTrue: [ frameScore := frameScore + self roll3 ].
    ^frameScore

And the tests still run. Now let’s move score and initializing frameStart to the #initialize method:

score
    10 timesRepeat: 
        [score := score + self frameScore.
        self updateFrameStart].
    ^score

initialize
    rolls := OrderedCollection new.
    score := 0.
    frameStart := 1.
    ^self

OK, I’m out of ideas. Let’s see what we think. Well, let’s see what I think, until someone writes in with other thinks.

Retrospective

All these changes went incredibly smoothly. Even though the refactoring browser wouldn’t always extract the methods for me, it was easy enough to select, copy, paste, to get what I wanted. So this was at least easy to do, if not good. And I rarely if ever broke the tests, other than by my standard Smalltalk error, forgetting to return the result.

But are these changes righteous?

If I have any concerns, they are with the instance variables:

Smalltalk defineClass: #BowlingGame2
    superclass: #{Core.Object}
    indexedType: #none
    private: false
    instanceVariableNames: 'rolls score frameStart '
    classInstanceVariableNames: ''
    imports: ''
    category: 'RonBowling'

We have rolls, the collection of rolls, and the score, which is the running total score, and frameStart, which is the starting index of each frame. In the original, score and frameStart were temps in the score method, while rolls was a parameter.

An issue with these variables is that they don’t all change at the same time. Score and frameStart change repeatedly during scoring. A case can be made that they are recording scoring state, and state is what objects are about. But rolls? That gets set separately, and stays constant all during scoring. This is a bit of a smell. In order to extract the various allegedly meaningful ideas like roll1, roll2, roll3, and whether the frame is a strike or spare, there’s not much choice.

When we see this happening – instance variables with different life spans or change patterns – it’s a sign that our class might not be cohesive, and that there may be two or more classes wanting to emerge. In this case, I wouldn’t go there, but I might be wrong.

But what about all these tiny methods? C-ish programmers are driven mad by them. But this was the way I was taught to do it in Smalltalk, and I was taught by the best. (I don’t claim to have become the best from all that teaching, but I try.) So my take on it is that this is closer to how the procedural solution would be expressed in Smalltalk. Note that it’s entirely the same algorithm, just broken out into tiny methods.

Now there is one more thing. Michael Lucas-Smith has been emailing with me about those irritating constants. (10, in this case.) If I’m channeling him correctly, he would write, for example:

score
    self numberOfFrames timesRepeat: 
            [score := score + self frameScore.
            self updateFrameStart].
    ^score

numberOfFrames
    ^10

I think that’s generally a good idea, especially since there’s another ten that appears in a few places, which we might resolve this way:

strike
    ^self roll1 = self allPinsDown

spare
    ^self roll1 < self allPinsDown 
        and: [self roll1 + self roll2 = self allPinsDown]

allPinsDown
    ^10

I think this is good advice. My main reason is that it’s more expressive. I’ve gotten pretty cavalier with tens in bowling, since I just about have this application memorized. Yet there are two different ideas with value ten in the program. That should be expressed. It also makes the game easier to modify for some new number of frames or pins, but I’m not very worried about that.

Hmm, what else? Well, I don’t like the name roll1, because it’s hard to tell from ro11l or rol1l. I might go to firstRoll, secondRoll, thirdRoll for that reason.

Otherwise, I think this little baby has been polished down to a fine luster, perhaps too fine. What do you all think?

Oh … and should I provide a full listing at the end of these things, or not? Drop me a note, with your vote!