up vote 2 down vote favorite

I'm refactoring an old gateway and came across the following method:

def apply_limit_and_offset(options)
  limit  = options[:limit]
  offset = options[:offset]

  @result = if offset
    @result.drop offset
  else
    @result
  end

  @result = if limit
    @result.first limit
  else
    @result
  end

  @result
end

@result is an Array of objects.

Any suggestions on how to refactor this? It works just fine, but multiple if statements make me feel gross.

share|improve this question
up vote 7 down vote accepted

Some notes:

  • Be consistent with the use of parens.

  • Just using ternary expressions instead of an if would reduce the line count a lot.

  • If you are using Ruby >= 2.0, check named keywords.

  • The resulting expression of an assignment is its value, so you can remove the last @result

  • What really looks gross to me are those in-place updates of @result, but this goes beyond the scope of the question.

With Ruby >= 2.0, I'd write:

def apply_limit_and_offset(limit: nil, offset: nil)
  @result = offset ? @result.drop(offset) : @result
  @result = limit ? @result.first(limit) : @result
end
share|improve this answer
    
named keywords are really nice, i had no idea about them. thanks! – dax Apr 13 '15 at 17:48
    
That's some pretty code; nice and elegant. +1 – Evan Bechtol Apr 13 '15 at 17:57
    
You could also write the first line, @result.shift(offset) if offset. – Cary Swoveland Apr 14 '15 at 2:30
up vote 3 down vote

Your operation is basically equivalent to Array#slice, so that is how I would write it.

def apply_limit_and_offset(options)
  @result = @result.slice(options[:offset] || 0,
                          options[:limit] || @result.length) || []
end

#slice appears to tolerate an options[:limit] that goes beyond the end of the array. However, if options[:offset] is out of range, then #slice returns nil, so we need to add || [] at the end to make it an empty array.

share|improve this answer
    
Very nice, but where's your usual thorough explanation? Why does this work for various case? For example, what if offset >= @result.size or 0 <= offset < @result.size and offset + limit >= @result.size? – Cary Swoveland Apr 14 '15 at 2:46
    
@CarySwoveland Indeed, the original version was all wrong. – 200_success♦ Apr 14 '15 at 3:10

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.