Multiplayer bowling in Ruby (follow-up: injection, single responsibility)

Question

This is a multiplayer bowling simulator in Ruby. It allows for variable skill levels, and produces weighted random results for each player's rolls based on those skill settings.

This is a complete rewrite of code I first posted for review here. My first solution was entirely procedural (all methods, no classes). I got some great pointers on OOP basics in the response, plus a referral to Sandi Metz, and worked out a new solution on that basis.

Everything works correctly. I'm posting the core classes, but the complete code (with scoring procedures, user input/screen output, tests) is in this gist.

For review: I'm looking for critiques/advice regarding the PlayerGame class in particular (3rd block below), where I've located the primary game logic.

Two basic questions:

1. Does this PlayerGame class qualify as 'single responsibility'? If not, what should go?
2. For a program this small, are the direct dependencies (see .bowl and .score_game - instance creation in each case) worth injecting or otherwise minimizing?

The game logic had to go somewhere, and PlayerGame seemed like the best place. The remaining classes are fairly well isolated/dumb. That means PlayerGame is directing instance creation; the dependencies are at least isolated in private methods, but it seems like I could/should do better.

I'm wondering about a) Moving instance creation outside the class entirely (I think this would involve learning more about factory patterns. In practice, though, would you typically bother with the extra layer in a program this small?); b) Removing the turn-by-turn score data from PlayerGame and encapsulating it in a different class (Or do frames-played and scores really belong in a common object?); and/or -- c) Rethinking the modeling entirely (Trying to get away from gameplay as instance creation?).

Design/modeling considerations:

• I'm defining the problem around a game that's scored in progress (as needed by the display at an alley, essentially), so this differs from the 'kata'/Robert Martin version of the problem.
• I'm modeling a Game as a collection of PlayerGame(s), and modeling an individual player-game as a series of consecutive Frame instances with corresponding subtotals.
• Frame results are generated by Player instances (which store and apply skill levels). Scores are calculated within one-time-use ScoringWindow instances that are only aware of a given player's last three frames. Keeping Frame as a class means .strike? (etc.) is available both during gameplay and during scoring.
• I'm modeling gameplay itself in terms of instance creation. Unlike card games (where you can generate a deck in advance, and gameplay is just re/distributing cards), it would seem odd to generate a bowler's frames in advance rather than on a turn-by-turn basis.

Player:

# This class creates arrays of weighted-random rolls (1 or 2, from 10 pins).
class Player
 attr_reader :skill 
 def initialize(skill = 0)
 raise RangeError unless skill >= 0
 @skill = skill
 end
 def roll
 reset_lane
 weighted_roll(@pins_standing)
 @results
 end
 private 
 def reset_lane
 @roll_no = 1
 @pins_standing = 10
 @results = []
 end
 def update_lane(pins_hit)
 @roll_no += 1
 @pins_standing -= pins_hit
 end
 def weighted_roll(pins)
 pins_hit = apply_skill(pins)
 @results << pins_hit
 update_lane(pins_hit)
 weighted_roll(@pins_standing) unless (pins_hit == 10 || @roll_no > 2)
 end
 def apply_skill(pins)
 picks = []
 (@skill + 1).times { picks << rand(0..pins)
 break if picks.max == pins }
 picks.max
 end
end

Frame:

# This class stores and evaluates a single array of rolls.
class Frame
 attr_reader :results 
 def initialize(player_roll)
 @results = player_roll 
 end 
 def first_roll
 @results[0]
 end
 def second_roll
 @results[1]
 end
 def total
 @results.reduce(:+) 
 end 
 def strike?
 @results[0] == 10
 end
 def spare?
 strike? == false && total == 10
 end
end

PlayerGame:

# This class generates and stores Frame objects, then sends for scoring.
class PlayerGame
 attr_reader :player, :frames, :scores
 def initialize(player) 
 @player = player
 @frames = []
 @scores = []
 end
 def take_turn
 @frames.length == 9 ? bowl_tenth : bowl
 score_turn
 end
 def frames_played
 @frames.map { |fr| fr.results }
 end
 def scores_posted
 @scores.flatten
 end
 private
 def bowl
 player_frame = Frame.new(@player.roll) 
 @frames << player_frame 
 end
 def bowl_tenth
 base_frame = Frame.new(@player.roll)
 if base_frame.strike? || base_frame.spare?
 tenth_frame = generate_bonus(base_frame)
 @frames << tenth_frame
 else
 @frames << base_frame
 end
 end
 # Covers all possible cases starting from strike or spare (including 10-10-10).
 def generate_bonus(base_frame)
 first_bonus = Frame.new(@player.roll)
 second_bonus = Frame.new(@player.roll)
 source_rolls = [base_frame.results, first_bonus.results, second_bonus.results]
 three_rolls = source_rolls.flatten.shift(3)
 Frame.new(three_rolls)
 end
 def current_frame
 @frames.length
 end
 def last_known_score
 @scores.compact.empty? ? 0 : @scores.compact[-1]
 end
 def score_turn
 active_frames = (current_frame <= 3) ? @frames : @frames[-3..-1]
 window = ScoringWindow.new(active_frames, last_known_score)
 @scores << window.return_scores[-1]
 @scores[-2] ||= window.return_scores[-2] if window.return_scores.length >= 2
 @scores[-3] ||= window.return_scores[-3] if window.return_scores.length == 3
 end
end

ScoringWindow:

Omitted here (see link above), but each instance generates/returns array of 1-3 elements, used by PlayerGame at end of preceding block.

Game:

# This class creates a single game and directs player(s) to bowl in sequence.
class Game
 attr_reader :players, :player_games 
 def initialize(players, turn_recorder)
 @players = players
 @turn_recorder = turn_recorder
 @player_games = []
 @players.each { |player| @player_games << PlayerGame.new(player) }
 play_game
 end
 private
 def play_game
 10.times { play_turn; record_turn }
 end
 def play_turn
 @player_games.each { |curr_player| curr_player.take_turn }
 end
 def record_turn
 @turn_recorder.record(@player_games)
 end
end

• Flambino: Very nice update!
• a2bfay: Thanks - learned a TON. The poker challenge was a great model; the CR/meta guides for asking questions and follow-up posts were also really helpful.
• 200_success: I've taken the liberty of removing your beginner tag. You may take that as a compliment.
• Flambino: @a2bfay In the last review, I threatened to write an implementation myself, and now it's here... well, it's only one class, actually, not a complete project. I started a while back, but didn't get it done until now. And I ran with a very different approach; I'm not trying bowl over your code. (I sincerely apologize for that pun.)
• a2bfay: @Flambino Glad to have it. Finally have the votes to offer a little something for this one as well

Answer 1

Ok, I had to review this :)

First of all: Wow. This is leaps and bounds beyond the first version. Heck, it's not even in the same category (literally; the previous version was procedural, this is object oriented. And has tests!). I am very impressed!

To try to answer your specific questions up front:

Does this PlayerGame class qualify as 'single responsibility'? If not, what should go?

Not quite. It and ScoringWindow are sort of stepping on each other's toes, but so are Player and Frame. It's tough to say what specifically should go, though, without knowing where it should go to. You can refactor things in thousands of ways, so I'd rather leave it open. Perhaps you'll get some refactoring ideas from the stuff below, though.

For a program this small, are the direct dependencies (see .bowl and .score_game [you meant score_turn, right?] - instance creation in each case) worth injecting or otherwise minimizing?

Size ain't got nothing to do with it. But really, don't worry about creating instances; that's what the new method is for. If your code is intended to create some frames, then by all means let it create some frames! You can't perfectly decouple everything - in fact what makes much of any code work is that it relies on other code. So it's about picking your battles. And in this case, I'd say you've picked well: turn_recorder is injected, while you create instances of Frame and ScoringWindow as needed. The former isn't integral or core to your model, so yeah, inject that. The latter two are integral to your model, so it makes sense depend firmly on those.

Review time.

I looked at the code in the gist, just to get a complete picture, so I've included a few (superficial) notes on ScoringWindow too, but I've left out the TurnRecorder class. But kudos on separating such user interaction code from the rest!

I've intentionally kept my notes either super low-level (syntax stuff) or more high-level (class interactions etc.). What's in between is refactoring, but that's an exercise left to the reader.

Overall notes

• There's quite a smattering of "magic numbers" across the classes. Most notably, of course, is the number 10. Constants or methods should help clean this up.

• As a trivial style note: It's funny that you've indented private one extra space. Most often it's just at the same level of indentation as whatever's around it, though other prefer to outdent it like you would else in a if..else..end statement. Personally, I do the former, but as I said: Trivial style note. There's a lot of code to get to.

Player

• The methods reset_lane and update_lane smell a bit as though Player has multiple responsibilities. It probably is overkill to make a separate Lane class, but from a semantics standpoint, it's perhaps a little strange that a player determines how many pins are standing. The player is solely in charge of saying when "its" turn is over.

• Use do..end for multiline blocks. In other words, please don't do this:

  (@skill + 1).times { picks << rand(0..pins)
   break if picks.max == pins }
  

  You could also use a semicolon instead of a line break, but this isn't the place for that either.

• Trivial small things:

  • Use @results.first instead of @results[0] in #strike?
  • I'd write the expression in #spare? using !strike? rather than the direct comparison with false

Frame

• If you have accessors (attr_* declarations, or custom accessor methods) it's often a good idea to rely on them within a class too. For instance, you could use the accessor for results in a couple of places, in place of the "raw" instance variable. This decouples your code internally.

• Addendum to the above: Take care when using auto-generated accessors; sometimes it's best to write your own, and make sure it returns a duplicate of your instance variable. For instance, right now, I could call some_frame.results and then modify the returned array. Since that array is the very same object as @results inside your class, I'm modifying the frame's internal data. There's no need to be paranoid about this, though (Ruby doesn't have a hardline approach to data/method access like some other languages do, so you can go crazy trying to lock everything down). Still, writing a method that returns @results.dup isn't too terrible, and it would avoid instance variables being modified by accident outside the instance.

PlayerGame

• You can use a short-hand syntax for your mapping in #frames_played:

  @frames.map(&:results)
  

  which reads as "call results on each item in the array". Same as how you use inject(:+) elsewhere to calculate a sum without writing out the entire block.

• I see Frame.new(@player.roll) in a bunch of places. This could be extracted into a method, and DRY your code (and, as a side-benefit, ease testing).

• Or, perhaps it's Player that should use Frame. Right now, Frame knows stuff like #strike? and #spare?- logic which is almost duplicated in Player as pins_hit == 10. Compared to the duplication of Frame.new(...) mentioned above, this is more troublesome, as the (near-)duplication spans multiple classes.

• In the tenth frame, you're actually playing 3 frames, then possibly discarding some rolls. While it certainly works, it does seem a little inelegant. I think you're perhaps conflating the concept of "frame" with that of a "roll". A game is divided into successive frames, but scoring is actually based on successive rolls.

• I mentioned magic numbers above, and while that usually pertains to actual numbers, a method name like #bowl_tenth is also a magic number in a sense. Or, at any rate, it's "magically numbered". But it's not horrible. In my own code, I hand-waved my own use of magic numbers by saying "well, I'm only interested in regular 10-pin/10-frames bowling". Which is a fair reason, but it's also fair to wag a finger at it.

• #score_turn (see below)

ScoringWindow

• This class and PlayerGame#score_turn has a tangled little web of intrigue. #score_turn digs into ScoringWindow's data a bit, and ScoreWindow, in turn, is created based on data in a PlayerGame. It's not terrible, and it's not quite a mutual dependency, but it is a little... intimate, for lack of better word. I do like the concept of a "scoring window", though.

• Overall, this class' methods smell a little complicated to me (almost every method has if..else branches, and/or early returns), and #update_two_prev and #update_one_prev seem to share some code that could be extracted.

Game

• Good use of dependency injection (turn_recorder)!

• You could define @player_games as simply @players.map { |player| PlayerGame.new(player) } instead of creating an array, and then pushing items to it in an each-block. Doing the latter is almost an anti-pattern in Ruby, when you have all of Ruby's lovely Array and Enumerable methods on your side.

A quick note on tests

Firstly: Yay, tests! Awesome job! ... yeah, that's basically my whole point here.

Oh, okay, one note: You have this comment on your tests for PlayerGame

Seems tough to test - game logic is here, with non-deterministic results, and requires contact with all classes except Game.

Indeed. It's not quite a code smell, since PlayerGame is a class that ties a lot of other classes together, and provided you can test those other classes fairly independently, you're doing ok. However, the non-deterministic part is a bit tricky. It makes it hard to trust your own tests, which isn't a good feeling.

It's great if you can somehow write your code to avoid such situations (without, of course, writing your code specifically for your tests; it should be the other way around). Also great if you can selectively stub out methods, and replace them with deterministic versions.

This, incidentally, is another reason why you'll often want to rely on accessors, rather than raw instance variables, inside your classes: It lets you stub out the accessor in your tests. This approach could perhaps have let you do some further testing of PlayerGame without nasty randomness.

---

With all that out of the way, I'm back to my original statement: I'm impressed. There are probably an infinite number of ways to approach this; you reasoned about yours, and the reasoning is sound. Implementation has some rough edges, though, but it ain't half bad. Keep up the good work!

• a2bfay: .@Flambino This is great - thanks. Will take some time to finish reviewing/comparing, but everything makes sense on the first couple reads. I realized as the pieces came together that Frame, in this approach, was just barely a class - if you didn't sometimes need that fill ball in the tenth, all those methods could simply move into scoring - but keeping it separate helped me figure out how to start writing tests. Didn't expect you to do all the heavy lifting but really appreciate the second look.