How to go about refactoring a complex method which iterates through an array-CodePudding

I am currently writing a fairly simple bowling scorecard project in ruby and have got my code working in a way that I want it to. I was wondering how do I go about refactoring a complex (or at least complex to a fairly new programmer like myself) if/else statement where each part of the if/else relies on the iteration through an array. The array contains a list of frame objects, where I have a Frame class. My code is below:

def score
    @score = 0
    @frames.each_with_index do |frame, index|
      if @frames.last == frame
        add_frame_score(frame.score)
      elsif frame.strike?
        frame.score  = if @frames[index   1].strike? && @frames.last != @frames[index   1]
                         @frames[index   1].total   @frames[index   2].roll1
                       elsif @frames[index   1].strike? && @frames.last == @frames[index   1]
                         (@frames[index   1].roll1   @frames[index   1].roll2)
                       else
                         @frames[index   1].total
                       end
        add_frame_score(frame.score)
      elsif frame.spare?
        frame.score  = @frames[index   1].roll1
        add_frame_score(frame.score)
      else
        @score  = frame.total
      end
    end
    @frames.each { |frame| frame.score = frame.calculate_total }
    @score
  end

  private

  def add_frame_score(frame_score)
    @score  = frame_score
  end

I would very much appreciate any ideas or processes that you might think of to understand how I might tackle a problem like this now and in the future.

CodePudding user response：

For a "more simple code", maybe is a interesting tool the clean code practice (article about and i recommend the lecture of the clean code book). Your code can be partitioned in small pieces and with this, the code will be more easy to read and understand. I don't know the rules of bowling but i'll give an example of refactoring:

 if last_frame(@frames, frame)?
    add_frame_score(frame.score)
 elsif frame.strike?
    frame.score  = if fist_rule(index)
                      compute_score_first_rule(index)
                   elsif second_rule(index)
                       ...

functions refatored example:

def fist_rule(index)
  is_a_strick(index)? && next_frame_not_is_the_last?(index)
end

def is_a_strick(index)?
  @frames[index   1].strike? 
end

def next_frame_not_is_the_last?(index)
   @frames.last != @frames[index   1] 
end

def compute_score_first_rule(index) 
  @frames[index   1].total   @frames[index   2].roll1
end

CodePudding user response：

how do I go about refactoring ... [when] if/else relies on the iteration through an array.

Define classes and helpful methods. Many methods are trivial but they let me think about bowling not array indexes. The "hard stuff" will be 20x less hard if you do that. Having done that, you can do this:

class ScoreKeeper
  def initialize(new_game)
    @game = new_game
  end
  
  def score_frame   #assumes Game.current_frame
    if @game.prev_frame.spare?
         @game.prev_frame.score = 
           @game.prev_frame(@game.prev_frame).score   
           @game.prev_frame.rolls   
           @game.curr_frame.roll1
                              
    #turkey?
    if @game.current_frame.strike? && 
       @game.prev_frame.strike? && 
       @game.prev_frame(@game.prev_frame).strike?
         @game.prev_frame(@game.prev_frame).score = 30
                              
    if @game.curr_frame.open?
         @game.curr_frame.score = @game.prev_frame.score   @game.curr_frame.rolls 
  end
end #ScoreKeeper

Now you can iterate a game, not a stupid (literally) Array.

P.S. A 'turkey' is a special case of the general rule for scoring a strike. score_frame can be fixed to cover the general rule without knowing anything about - or modifying! - any other classes in the program. Behold the invisible hand of Object Oriented programming!