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..

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