Multiplayer bowling in Ruby, with variable skill

Question

This is a multiplayer bowling simulator in Ruby. It allows for variable skill levels, and produces weighted random results for each roll based on those skill settings. The methods are grouped in four sections:

1. game mechanics
2. scoring calculations
3. stats calculators (because I got curious about what my skill/weighting system was generating)
4. gameplay options.

The game mechanics fill a nested array with the results of individual rolls. The scoring operates on those to fill a second array with the frame-by-frame scores. The screen outputs are scattered through the various methods, but they get turned off when you run multiple games to check stats.

Everything works correctly. The code is also here.

I'm aware of the more straightforward 'kata'/Robert Martin version of the bowling problem (where you do all the rolls first and only then score the complete game). But I first encountered the problem in this form -- a game that's scored in progress, like an actual bowling alley display -- and wanted to solve it this way, along with the additional challenges noted above.

I'm new to Ruby and to coding, and experimenting on my own. I haven't figured out 'unit tests' yet so there aren't any. I also suspect this could/should all be wrapped in a single class, but haven't had a chance to work that out. If that's necessary to make the code reviewable, please let me know.

I'd appreciate critiques/advice on any of the following:

1. obvious rookie mistakes or bad habits to nip in the bud
2. proper use of instance/local variables
3. use of arrays to store player/game data

4. definition and length of methods, relative to either design logic or general readability

   (Some I couldn't keep under 10 lines, given the number of cases/conditionals to sort through, but I tried to follow the short-method recommendation as much as possible. The scoring methods definitely need improvement, but I'm interested in where else additional refactoring is warranted.)

5. order of methods within code

   (I know it doesn't affect execution, but how much does it matter for readability?)

6. The system I'm using for weighted rolls (line 120 in roll_calc)

   (I considered rand(0..pins_remaining + skill), then rounding down anything over the number of pins; it's low-cost and makes it easy to increase the number of strikes, but otherwise an 8 is as likely as a 2 -- not a good representation. I also considered taking the nth-root of rand(o..pins_remaining**n), which makes higher numbers proportionally more likely for a better bowler, in a nice way, but seemed really costly.)

7. Where I'm furthest off the mark in terms of Ruby style/standards

Game mechanics:

# ===============================================
# GAME MECHANICS
#
def newgame
 @players = Array.new
 @frame_scores = Array.new
 @game_scores = Array.new
 @player = 1
 @frame_no = 1
 @roll_type = 1
 @pins_remaining = 10
end
def get_players
 print "How many players(1-4)? "
 numplayers = gets.chomp.to_i
 numplayers = 1 if numplayers < 1
 numplayers = 4 if numplayers > 4
 get_skill(numplayers)
 reset_arrays
 print_players
end
def get_skill(numplayers)
 puts "Enter skill level 0-15 (2+ = good, 4+ = v.good, 6+ = pro):"
 (1..numplayers).each do |p|
 print "Skill level for player #{p}? "
 skill = gets.chomp.to_i
 skill = 0 if skill < 0
 @players << skill
 end
end
# prepares nested arrays to be filled during gameplay/scoring
#
def reset_arrays
 @players.length.times do
 @frame_scores << []
 @game_scores << [] 
 end
end
def print_players
 puts
 ([email protected]).each do |p|
 print "P#{p}(#{@players[p - 1]})".rjust(7), "___________ "
 end
 puts
end
def playgame
 until @frame_no == 11
 roll_control(@player) 
 end
 puts unless @stats_mode == true
end
def turn_control(player)
 if player == @players.length
 puts unless @stats_mode == true
 @frame_no += 1
 @player = 1
 else
 @player += 1
 end
 @roll_type = 1 
 @pins_remaining = 10 
end
def roll_control(player)
 if @roll_type == 4 # for 10th/3rd
 roll_calc(player,1)
 score_progress(player,@frame_no - 1) 
 turn_control(player) 
 elsif @roll_type == 3 # 1st bonus after 10th fr strike -> to 3rd roll
 roll_calc(player,1)
 @pins_remaining = 10 if @pins_remaining == 0
 @roll_type = 4 
 else
 roll_calc(player,@roll_type - 1) 
 if @roll_type == 1 # regular first roll
 if @pins_remaining == 0 and @frame_no == 10
 @roll_type = 3 
 @pins_remaining = 10
 elsif @pins_remaining == 0
 score_progress(player,@frame_no - 1)
 turn_control(player)
 else
 @roll_type = 2 
 end
 elsif @roll_type == 2 # regular second roll
 if @pins_remaining == 0 and @frame_no == 10 
 @roll_type = 4 
 @pins_remaining = 10
 else
 score_progress(player,@frame_no - 1)
 turn_control(player)
 end
 end 
 end 
end
def roll_calc(player,type)
 skill = @players[player - 1]
 picks = Array.new
 (skill + 1).times do 
 pins = rand(0..@pins_remaining)
 picks << pins
 break if pins == @pins_remaining
 end
 pins_hit = picks.max 
 if type == 0 
 @frame_scores[player - 1] << [pins_hit]
 else 
 @frame_scores[player - 1][@frame_no - 1] << pins_hit
 end
 @pins_remaining = @pins_remaining - pins_hit
end

Scoring methods:

# ===============================================
# SCORING METHODS
# "i" in next 3 methods is array index of current frame (so @frame_no - 1)
# should be more descriptive, but lines are already too long --
# better to create variables for array values being summed, then add? 
#
def score_progress(player,i)
 framesum = @frame_scores[player - 1][i].reduce(:+)
 if i >= 2 && @game_scores[player - 1][-2].nil? == true
 update_2prev(player,i)
 end
 if i >= 1 && @game_scores[player - 1][-1].nil? == true
 update_prev(player,i,framesum)
 end
 if i == 9 
 @game_scores[player - 1] << @game_scores[player - 1][-1] + framesum
 elsif framesum == 10
 @game_scores[player - 1] << nil
 elsif i == 0
 @game_scores[player - 1] << framesum
 else 
 @game_scores[player - 1] << @game_scores[player - 1][i - 1] + framesum
 end
 unless @stats_mode == true
 print @game_scores[player - 1][i].to_s.rjust(6)
 print " ", @frame_scores[player - 1][i].inspect.ljust(12)
 end
end
def update_2prev(player,i)
 if i == 2
 @game_scores[player - 1][-2] = 20 + @frame_scores[player - 1][i][0]
 else
 @game_scores[player - 1][-2] = @game_scores[player - 1][-3] + 20 + @frame_scores[player - 1][i][0]
 end 
end
def update_prev(player,i,framesum)
 if @frame_scores[player - 1][i - 1][0] == 10 && @frame_scores[player - 1][i][0] == 10
 if i == 9
 @game_scores[player - 1][-1] = @game_scores[player - 1][-2] + 10 + @frame_scores[player - 1][i][0] + @frame_scores[player - 1][i][1]
 end
 return 
 elsif i == 1 
 if @frame_scores[player - 1][0][0] == 10
 @game_scores[player - 1][0] = 10 + framesum
 else
 @game_scores[player - 1][0] = 10 + @frame_scores[player - 1][1][0]
 end
 else
 if @frame_scores[player - 1][i - 1][0] == 10
 if i == 9 
 @game_scores[player - 1][-1] = @game_scores[player - 1][-2] + 10 + @frame_scores[player - 1][i][0] + @frame_scores[player - 1][i][1] 
 else
 @game_scores[player - 1][-1] = @game_scores[player - 1][-2] + 10 + framesum
 end
 else
 @game_scores[player - 1][-1] = @game_scores[player - 1][-2] + 10 + @frame_scores[player - 1][i][0]
 end
 end
end

Stats tests:

# ===============================================
# STATS TESTS
#
# average score by skill setting
#
def average_test(limit)
 (0..15).each do |skill|
 gamecount = 0
 pintotal = 0
 minscore = 300
 maxscore = 0
 until gamecount == limit
 newgame
 @players << skill
 reset_arrays
 playgame
 gamecount += 1
 pintotal += @game_scores[0][-1]
 minscore = @game_scores[0][-1] if @game_scores[0][-1] < minscore
 maxscore = @game_scores[0][-1] if @game_scores[0][-1] > maxscore
 end
 print "#{skill}\t#{gamecount}\tavg"
 print "#{(pintotal / gamecount)}".rjust(4), "#{minscore}/#{maxscore}".rjust(9), "\n"
 puts
 end
end
# frequency of perfect game by skill setting
#
def perfect_test(limit)
 (0..15).each do |skill|
 gamecount = 0
 perfect_games = 0 
 until gamecount == limit
 newgame
 @players << skill
 reset_arrays
 playgame
 gamecount +=1 
 perfect_games += 1 if @game_scores[0][-1] == 300 
 end
 print skill, "\t", gamecount, "\t", perfect_games, "\n"
 end
end
# head-to-head wins for consecutive skill levels
# (n.b. diff greatest at low skill - approach 50/50 at high)
#
def upset_test(limit = 1)
 puts
 (0..14).each do |skill|
 wins = [0,0,0]
 gamecount = 0
 until gamecount == limit
 newgame
 @players << skill
 @players << (skill + 1)
 reset_arrays
 playgame
 wins[0] += 1 if @game_scores[0][-1] > @game_scores[1][-1]
 wins[1] += 1 if @game_scores[0][-1] < @game_scores[1][-1]
 wins[2] += 1 if @game_scores[0][-1] == @game_scores[1][-1]
 gamecount += 1
 end
 print @players.inspect.rjust(8), " ", wins.inspect, "\n"
 end
end

Play options:

# ===============================================
# PLAY MODES:
#
def stats_tests(limit)
 @stats_mode = true # use to disable gameplay screen output
 puts
 average_test(limit)
 perfect_test(limit)
 upset_test(limit) 
end
def repeat_check
 puts "\nRepeat game with these players (y/n/stats/quit)?"
 repeat = gets.chomp.downcase
 if repeat == "stats"
 print "Number of games per test? "
 limit = gets.chomp.to_i
 stats_tests(limit)
 elsif repeat == "n"
 single_game
 elsif repeat == "y"
 curr_players = @players
 newgame
 @players = curr_players
 reset_arrays
 print_players
 playgame
 repeat_check
 else
 return
 end 
end
def single_game
 newgame
 get_players
 playgame
 repeat_check
end
single_game

Answer 1

Nice question! I may have a go at writing an implementation myself and put it up for review; it's a fun challenge.

Review-wise I haven't gone into great detail (there's a lot of code there), but here are some thoughts:

• Encapsulation. Right now, everything is happening in the global object. It's just one big soup of methods and data. Ruby is object-oriented, so you should create classes to encapsulate related data and methods. For instance, I might start by creating a Player class, which would model a single player (obviously). This class might also be responsible for tracking that player's skill level, score, frames bowled, and stuff like that (or perhaps that should be in a separate class?). A more high-level Game class could handle several players, and thus model the game itself, the order of players, and figure out the winner by examining the players' scores.
  This is just off the top of my head; another structure might be more appropriate. For instance, the Game class could just keep track of the frames, and the each frame could, in turn, reference the player that played it.
  It's all a question of how you describe (model) the game of bowling. Is it a game "where players play frames" or a game where "frames are played by players"? Maybe both? Domain modelling is hard, but plan a little, code a little, and see what works. Optimally using tests/specs.

• Separation of concerns. As you say "the screen outputs are scattered through the various methods". This is a red flag. For one, it's not DRY to repeat code (such as the code prompting the user for something) all other the place. For another, your classes (which I'll just assume you've created since reading the above), are ideally abstract clumps of data and logic. They shouldn't also be responsible for user interaction. Rather, you should separate the logic that specializes in user interaction, from the logic that does the scoring, from the logic that handles skill level, etc..

• Coupling. Right now, in part because stuff isn't encapsulated, you have a tangled web of dependencies. For instance, you have stuff like update_prev(player,i,framesum), which takes 3 arguments (and which could use some extra whitespace to breathe, but that's another story). That'd be fine if all the method did was work only on those three arguments. But 2 of those arguments are each indirect references to something else, since they're just array indices. So you jot down some index from the @players array and pass it to the method. Now, the method also has to know about, and access, the @players array. Both ends of the system are dependent on (coupled to) that array, but neither one is really responsible for it. It just sort of has to be there. It's a game of Jenga, where all the parts interlock, so the slightest touch will knock the whole thing over.
  Generally, if you've encapsulated stuff well, the various parts of the code know as little as possible about the rest of the system. For instance, a Player object doesn't know about prompting the user; it's just told its skill level. A Game object may not know anything about skill levels or frames, it just knows it's got, say, 3 player objects. And perhaps you have a Frame class, which doesn't know much about anything - it just stores (and perhaps generates) 2 numbers and a total. That's decoupling in a nutshell. "Compartmentalization" would be another word to describe it and encapsulation.

• Top-down design. This is more a way of approaching the code. Basically, it's easy to think "ok, I know that at some point I have to add up the number of pins knocked over" - and then start by coding that part and getting stuck in the minutiae and implementation details. It's easy because you get that part. That part you can solve now. But you quickly lose sight of what you're actually trying to answer: "Who won the game?" This is bottom-up design, and it's rarely the best way to do things. So, instead, try starting out by thinking "this is how I'd like to do X". For instance, in a bowling sim, I'd like to be able to say stuff like some_player.score and game.finished?. With building blocks like those, it's easy to figure out which player won the game. But of course it's all imaginary, since you haven't written the code yet. But you just pick one of those, and repeat the process. E.g. to figure out a player's score, it'd be nice to be able to say frame.score? for each frame and stuff like that. And then you delve into those methods and imagine the ideal way to write them. You continue until you hit bedrock; the level at which you're working with such a simple problem that you don't need more intermediary building blocks to help you. It's really not that different from writing an outline for a paper, before you start writing the content. It takes practice, and sometimes you code yourself into a corner, but eventually it'll come naturally. It's not that you have to slavishly follow this process, it's just important to stay cognizant of the big picture.

Alright, so that was the high-level analysis. Again, I didn't go through this line-by-line, but I plucked a few things I noticed. Note, though, that to really get somewhere, you'd have to take all the above into consideration first. Sure, you can make small tweaks, but it's (pardon the harsh phrase) lipstick on a pig.

• @frame_scores[player - 1]... you repeat this again and again. At least make a local variable like player_index = player - 1, and use that instead.

• A whole lot of inscrutable conditionals. I've already mentioned it, but something like

  if @frame_scores[player - 1][i - 1][0] == 10 ...
  

  makes be shiver. What about at least having a constant for a strike?

  if @frame_scores[player - 1][i - 1][0] == STRIKE ...
  

  or better yet, if you model a Frame as a class, you can give it a strike? method:

  if @frame_scores[player - 1][i - 1][0].strike? ...
  

  Ultimately, what I'd like to see is something like

  if player.last_frame.strike? ...
  

I could find more micro-level stuff, but again, the overall structure is the biggest bugbear here.

By the way, I highly recommend Sandi Metz's "Practical Object-Oriented Design in Ruby" as a great book for learning about all of this stuff.

And do look into unit testing. It's not just a skill, it's a habit - a good habit - to pick up. So pick it up early.

• a2bfay: That's great - thanks for the time and attention. I'm trying to learn this site as I try to learn some coding basics, so everything you've said is really instructive. Sounds like the higher-level stuff (classes, OOP, encapsulation) is the next thing I should tackle, and the smaller legibility changes can fall into place after the overall design has been improved. Looking forward to giving it a shot (though it'll take a little time before I can update with a rewrite). Metz looks like a great resource as well.
• Flambino: @a2bfay Glad it was useful! Note, though, that this whole bowling sim is actually quite a complex thing. Plenty of OOP to do, but I fear it might actually be a little overwhelming for a first try (I just tried doodling something myself, and it's surprisingly tricky!). Still, don't let me stop you from trying! And do check out Metz's book - it really is great for learning OOP techniques. As for accepting answers: Up to you! I'd wait a while, though; get more views and (hopefully) answers. (BTW, even once you've accepted an answer, you can always unaccept if something better comes along)
• Flambino: @a2bfay By the way, if you want, you can take a look at this old question of mine (and its review, and the comments) about scoring a hand of cards in a poker game. While it's nowhere near as complex as a bowling sim, I think it nicely illustrates how you can break code up into many small methods, and get something that's neat and clean.
• a2bfay: Ah, funny. Poker scoring was the first thing I wrote (unless you count fizzbuzz), but with zero concern for OOP - just trying to get the right things to appear on screen. I'm already seeing how the first chapters in Metz line up with your design-level comments, and it should be fun to rethink this one. I'll probably drop the repeat-game settings and average calculators for now, and try to rebuild a single-player/single-skill version with proper class definitions. Adding additional players and skill levels from there should be a good test of whether I have the basics down. @Flambino
• Flambino: @DamienWilson Yeah, rigorous testing is a great thing to learn - and good to learn early before other coding habits set in. I know I'd like to get better at TDD; I learned it late, so I tend to neglect it :/