Bowling scores in Ruby

Question

A little while back, I wrote in a review of a Ruby bowling sim that I might try my hand at modelling the game myself, focussing on the funky "deferred scores" system. Since a2bfay, who posted the original question, has since posted a thoroughly updated piece of code, I figured I'd better get mine finished, especially since I chose a very different approach. I just figured it'd be fun to put up for review. The main point for me was mostly to do some plain ol' Ruby for fun and zero profit. It's a self-imposed programming-challenge, I suppose.

This it not a complete bowling simulator like a2bfay's, though; it's actually just a single class for now. It does a lot, though. Perhaps too much, but that's for a review to tackle.

The class in question models the concept of a "frame" in a game of bowling. Since a frame's final score might depend on shots/rolls in the following frames, I implemented the frames as nodes in a singly-linked list. So at any time, a frame can report its score, and whether or not said score is final by examining itself and (if necessary) shots from its following frame(s). The final frame behaves differently, allowing up to 3 shots/rolls in order to resolve any non-final scores.

Basic requirements:

• Adherence to regular 10-pin bowling rules. No support for any of the variants.
• Ability to calculate scores "live", i.e. while the game is in progress.
• Ability to easily control "skill", i.e. how many pins to knock down.

I fairly happy with the result, though the class is pretty packed with functionality. For instance, here's how to play a perfect game:

 game = Frame.create_list(10).each do |frame| # create 10 linked frames
 frame.play { 10 } # bowl a strike in each frame
 end
 game.map(&:score).inject(&:+) # => 300

So it doesn't leave much for other classes to do¹. Not necessarily a bad thing, of course.

Anyway, enough intro, here's the code. It's also in a gist along with many, many tests (RSpec). Kinda went overboard, I guess, but I wanted to stretch my testing muscles.

# This class models a "frame" in 10 pin bowling.
class Frame
 # Number of pins lined up
 PIN_COUNT = 10
 # The next frame relative to the receiver
 attr_reader :next_frame
 # Create an array of +count+ linked frames, in which frame N is linked
 # to frame N+1 from first to last.
 def self.create_list(count)
 raise RangeError.new("count must be greater than 0") if count.to_i < 1
 frames = [self.new]
 (count.to_i - 1).times.inject(frames) do |frames|
 frames << frames.last.append_frame
 end
 end
 def initialize #:nodoc:
 @shots = []
 end
 # Returns a dup of the shots in this frame.
 def shots
 @shots.dup
 end
 # Append a new frame to the receiver (if one exists, it's returned).
 # If the receiver has already recorded 1 or more shots, no new frame
 # will be created, and the method will just return the existing
 # +next_frame+ which may be +nil+.
 def append_frame
 return @next_frame if last? && !shots.empty?
 @next_frame ||= self.class.new
 end
 # True if the receiver has been played to completion (i.e. no more shots
 # are possible).
 def played?
 shots.count >= shot_limit
 end
 # Play this frame till it's done. Returns the shots that were recorded
 #
 # See also #play_one_shot
 def play(&block)
 shots_played = []
 shots_played << play_one_shot(&block) until played?
 shots_played
 end
 # Bowl a single ball. If the frame's been played, +nil+ will be
 # returned.
 # 
 # The block will receive the number of pins still standing, and
 # the numbers of the shots already taken, and must return the number
 # of pins to topple. The number is clamped to what's possible.
 def play_one_shot(&block)
 return if played?
 shot = yield remaining_pins, shots.count
 @shots << [0, shot.to_i, remaining_pins].sort[1]
 shots.last
 end
 # True if the receiver has no following frames (see also #next_frame).
 def last?
 next_frame.nil?
 end
 # The score for this frame (may not be final).
 def score
 scored_shots = successive_shots[0, shots_required_for_score]
 scored_shots.inject(0, &:+)
 end
 # True if have all the necessary shots been taken (in this frame or others).
 def score_finalized?
 successive_shots.count >= shots_required_for_score
 end
 # The number of pins knocked down in the shots that have been taken.
 def pins
 shots.inject(0, &:+)
 end
 # True if the first shot was a strike.
 def strike?
 shots.first == PIN_COUNT
 end
 # True if the first 2 shots constitute a spare.
 def spare?
 !strike? && shots[0, 2].inject(0, &:+) == PIN_COUNT
 end
 # Shots required in order to calculate the final score for this frame.
 # This includes shots already taken this frame.
 def shots_required_for_score
 strike? || spare? ? 3 : 2
 end
 # Returns the number of pins currently standing.
 def remaining_pins
 remaining = PIN_COUNT - (pins % PIN_COUNT)
 remaining %= PIN_COUNT if played?
 remaining
 end
 # Collect shots from this frame and successive frames.
 def successive_shots
 return [] if @shots.empty?
 collected = @shots.dup
 collected += next_frame.successive_shots if next_frame
 collected
 end
 private
 # The number of shots that can be taken in this frame.
 def shot_limit
 return 3 if fill_ball?
 return 1 if strike?
 return 2 # intentional but unnecessary return; just there for formatting
 end
 # Does this frame have a 3rd shot?
 def fill_ball?
 last? && (strike? || spare?)
 end
end

Things I've noticed myself:

• Conflating a "regular" mid-game frame and the last frame in a single class. They do differ a fair bit. But while I tried breaking it into separate classes, super and sub classes, and extracting logic into modules, I didn't find a structure I liked. So it's all one class.

• There are a handful of "magic numbers" in there, but creating constants or methods for them seemed more trouble than it's worth, since I'm aiming exclusively at regular 10-pin bowling. Still, though...

• Again, the class just does a lot. Hmm... it is kinda complex, isn't it?

---

Edits & addenda:

Edit: Added a missing comment on Frame.create_list

¹) I should have been clearer. I do not intend for this single class to do and be everything, even if it does do a lot. To more fully model a game, you'd still need something like a Game class, perhaps a Player class, etc..

• a2bfay: Ok - so the block you pass to .play and on to play_one_shot is where you'd control for skill, yes? (And .create_list is a class method, so you don't have to make some other class directly responsible for generating instances - got it.)
• Flambino: @a2bfay Yup, the block is where "skill" comes in. The block can be as simple or as complex as you'd like; in the end it just has to return a number. And yeah, create_list is a factory method. Typically, it's fine to just let others create instances of your class, but in this case, frame instances are very much intended to be linked to each other to form a list. It makes sense to me that the logic for doing so is present in Frame (just for ease of use if nothing else).

Answer 1

Modelling

The Frame class alone is insufficient to model a game. Chaining frames in a linked list also adds complexity, especially since those frames also reside in an array.

A BowlingGame class should exist. A static method create_list(count) feels too informal a substitute for BowlingGame.new. Not to mention, methods such as #completed? and #score would be handy for anyone wanting to use this code. Furthermore, it is cumbersome to require the caller to keep track of which frame to call #play on.

Instead of having a frame reach into one or two of its successors to extract bonus shots, I think it would be cleaner to have the game broadcast a message to all not-yet-finalized frames to update their scores.

The interface feels cluttered. A lot of tiny methods, such as #last?, #shots_required_for_score, #successive_shots, #shot_limit, and #fill_ball? need not exist. To some extent, these methods aid readability, but there are just too many of them, and I find it hard to mentally trace the flow of control as a result.

I am puzzled by the modulo operations in #remaining_pins. I would simplify it by redundantly keeping track of the number of remaining pins, rather than trying to deduce it from the shots array.

Error handling

You do some validation. For example, Frame#create_list can throw a RangeError.

However, #play_one_shot seems to be too accommodating by clamping the shots between 0 and remaining_pins. (Using the median of [0, shot.to_i, remaining_pins] is rather cute, I think.) If you detect an impossible shot, why not throw an exception?

It's not clear how the game ends. I believe it's possible to replay the same frame over and over again until you get a score that you like. ☺

Suggested implementation

class BowlingGame
 # Number of normal frames in a game. (Bonus frames may follow.)
 FRAME_LIMIT = 10
 # Number of pins lined up
 PIN_COUNT = 10
 class BowlException < Exception ; end
 class Frame
 attr_reader :remaining_pins, :shots
 def initialize(frame_num)
 @frame_num = frame_num
 @remaining_pins = PIN_COUNT
 @shots = []
 end
 # Do not use this method directly; call BowlingGame#play instead.
 def register_shot(pins)
 if !played?
 @remaining_pins -= pins
 end
 if !score_finalized?
 @shots << pins
 end
 end
 def score
 @shots.inject(0, :+)
 end
 def strike?
 shots.first == PIN_COUNT
 end
 def spare?
 !strike? && remaining_pins == 0
 end
 # Returns true if a strike or two shots have been played.
 def played?
 remaining_pins == 0 || shots.length == 2
 end
 # Returns true if the score has been finalized: no more bonus shots
 # need to be considered.
 def score_finalized?
 if @frame_num > FRAME_LIMIT
 played? # No more bonuses
 else
 shots.length == ((strike? || spare?) ? 3 : 2)
 end
 end
 def to_s
 strike? ? 'X' :
 spare? ? "#{shots[0]}/" :
 shots[0...2].join()
 end
 end
 attr_reader :frames
 def initialize
 @frames = []
 new_frame()
 end
 def play(&block)
 throw BowlException.new("Game already completed") if completed?
 pins = yield frames.last.remaining_pins, frames.last.shots.length
 throw BowlException.new("Impossible shot") if pins > frames.last.remaining_pins
 frames.each { |f| f.register_shot(pins) }
 new_frame() if frames.last.played? && !completed?
 end
 def completed?
 frames.length >= FRAME_LIMIT && frames[FRAME_LIMIT - 1].score_finalized?
 end
 def score
 frames[0...FRAME_LIMIT].collect { |f| f.score }.inject(0, :+)
 end
 def to_s
 frames.collect { |f| f.to_s }.join('-')
 end
 private
 def new_frame
 @frames << Frame.new(@frames.length + 1)
 end
end

• Flambino: I didn't make myself clear, sorry. I do not intend for this to be a substitute for a "game" class or anything else. As I said, it's not a complete bowling sim - just one class in isolation. When I wrote that Frame "doesn't leave much for other classes to do", I was alluding to the fact that other classes would be necessary but can be simple - not that they can be left out. I do agree that the interface is a little cluttered though, and I like the broadcast idea. By the way, you say it's possible to replay a frame until you're happy? Is it?
• Flambino: More comments: The modulo ops in #remaining_pins are indeed tricky, I admit. But for the last frame, the initial number of pins isn't 20 or 30 to begin with. It's 10 like any other frame - but unlike any other frame, the pins may be reset for the 2nd and 3rd shots. Since remaining_pins is what's passed to the "play block", it has to accurately reflect the number of pins still standing at that point in time.
• 200_success: I've modelled the endgame by adding up to two semi-fictitious frames, and only including the scores of the first ten frames.
• Flambino: Indeed, and it's a nice implementation. I was just explaining my reasoning. I chose to not employ any extra pseudo-frames, and make the last frame itself handle the endgame. The cost was increased complexity - that I admit