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

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:

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

• 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

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 `

StackExchange Code Review Q#66662, answer score: 3