CODEWITHSUNDEEP
rubyarrays
M. Cypher
M. Cypher

Reputation: 7066

How to rewrite this Ruby loop in a cleaner fashion

I'm implementing a loop in Ruby, but it looks ugly and I wonder if there's a neater, more Ruby-like way of writing it:

def get_all_items
  items = []; page = 1; page_items = nil
  while page_items != [] # Loop runs until no more items are received
    items += (page_items = get_page_items(page))
    page += 1
  end
  items
end

Note that the get_page_items method runs a HTTP request to get the items for the page, and there is no way of knowing the number of pages, or the total number of items, or the number of items for any page before actually executing the requests in order until one of them returns an empty item set.

Imagine leafing through a catalog and writing down all the products, without knowing in advance how many pages it has, or how many products there are.

Upvotes: 2

Views: 401

Answers (8)

Sony Santos
Sony Santos

Reputation: 5545

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

Upvotes: 0

Mladen Jablanović
Mladen Jablanović

Reputation: 44080

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]

Upvotes: 1

M. Cypher
M. Cypher

Reputation: 7066

The short version, just for fun ;-)

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

Upvotes: 1

coreyward
coreyward

Reputation: 80041

I think that this particular problem is compounded because A) there's no API for getting the total number of items and B) the response from get_page_items is always truthy. Further, it doesn't make sense for you to iteratively call a method that is surely making individual requests to your DB with an arbitrary limit, only to concatenate them together. You should, at the risk of repeating yourself, implement this method to prompt a DB query (i.e. model.all).

Normally when you are defining an empty collection, iterating and transforming a set, and then returning a result, you should be using reduce (a.k.a inject):

array.reduce(0) { |result, item| result + item } # a quick sum

Your need to do a form of streaming in this same process makes this difficult without tapping into Enumerable. I find this to be a good compromise that is much more readable, even if a bit distasteful in fondling this items variable too much:

items = []
begin
  items << page_items = get_page_items(page ||= 1)
  page += 1
end until page_items.empty?
items.flatten

Upvotes: 3

Mario Uher
Mario Uher

Reputation: 12397

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

Upvotes: 1

DigitalRoss
DigitalRoss

Reputation: 146053

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

Upvotes: 1

Jordan Running
Jordan Running

Reputation: 106027

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.

Upvotes: 1

asfallows
asfallows

Reputation: 6108

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.

Upvotes: 0

Related Questions