How to rewrite this Ruby loop in a cleaner fashion

Here's how I'd have written it. You'll see it's actually more lines, but it's easier to read and more Rubyish.

def get_all_items items = [] page = 1 page_items = get_page_items page until page_items.empty? # Loop runs until no more items are received items += page_items page += 1 page_items = get_page_items page end items end 

You could also implement get_page_items as an Enumerator which would eliminate the awkward page += 1 pattern but that might be overkill.

I don't know that this is any better, but it does have a couple of Ruby-isms in it:

def get_all_items items = []; n = 0; page = 1 while items.push(*get_page_items(page)).length > n page += 1 n = items.length end end 

I would use this solution, which is a good compromise between readability and length:

def get_all_items [].tap do |items| page = 0 until (page_items = get_page_items(page)).empty? items << page_items page += 1 end end end 

The short version, just for fun ;-)

i=[]; p=0; loop { i+=get_page_items(p+=1).tap { |r| return i if r.empty? } } 

I wanted to write a functional solution which would closely resemble the task you want to achieve.

I'd say that your solution comes down to this:

For all page numbers from 1 on, you get the corresponding list of items; Take lists while they are not empty, and join them into a single array.

Sounds ok?

Now let's try to translate this, almost literally, to Ruby:

(1..Float::INFINITY). # For all page numbers from 1 on map{|page| get_page_items page}. # get the corresponding list of items take_while{|items| !items.empty?}. # Take lists while they are not empty inject(&:+) # and join them into a single array. 

Unfortunately, the above code won't work right away, as Ruby's map is not lazy, i.e. it would try to evaluate on all members of the infinite range first, before our take_while had the chance to peek at the values.

However, implementing a lazy map is not that hard at all, and it could be useful for other stuff. Here's one straightforward implementation, along with nice examples in the blog post.

module Enumerable def lazy_map Enumerator.new do |yielder| self.each do |value| yielder.yield(yield value) end end end end 

Along with a mockup of your actual HTTP call, which returns arrays of random length between 0 and 4:

# This simulates actual HTTP call, sometimes returning an empty array def get_page_items page (1..rand(5)).to_a end 

Now we have all the needed parts to solve our problem easily:

(1..Float::INFINITY). # For all page numbers from 1 on lazy_map{|page| get_page_items page}. # get the corresponding list of items take_while{|items| !items.empty?}. # Take lists while they are not empty inject(&:+) # and join them into a single array. #=> [1, 1, 2, 3, 1] 

It's a small (and almost entirely cosmetic) tweak, but one option would be to replace while page_items != [] with until page_items.empty?. It's a little more "Ruby-ish," in my opinion, which is what you're asking about.

def get_all_items items = []; page = 0 items << page_items while (page_items = get_page_items(page += 1)) items end