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.