6
\$\begingroup\$

get_item_count calls to an API and gets a count. There are several pages to get this count from until there are no more results.

How do I improve this code to remove all of the extra variables and make it easier to read?

final_count = 0
page = 1
final_count = individual_count = get_item_count(config, page)
while individual_count > 20
  page = page + 1
  individual_count = get_item_count(config,page)
  final_count = individual_count + final_count
end
\$\endgroup\$
  • 2
    \$\begingroup\$ Do you want get_item_count() to be executed only once, that is, for page = 1 ? \$\endgroup\$ – Kinjal Sep 18 '13 at 5:54
  • 1
    \$\begingroup\$ Yes I do. I would prefer to only call it in once but in a loop. \$\endgroup\$ – Kieran Andrews Sep 18 '13 at 7:36
  • \$\begingroup\$ Yay, I almost forgot how is it to have a feeling that it can't be cleaned, but can't come with solution in less, than 5 minutes ) \$\endgroup\$ – Nakilon Sep 18 '13 at 9:31
  • \$\begingroup\$ Within the loop you call get_items(), but I assume you mean get_item_count(). (?) \$\endgroup\$ – toto2 Sep 18 '13 at 19:33
  • \$\begingroup\$ I updated the question with the error. \$\endgroup\$ – Kieran Andrews Sep 19 '13 at 2:29
2
\$\begingroup\$

Use a do-while loop, so the loop body gets always executed at least once:

final_count = 0
page = 1
begin
  individual_count = get_item_count(config, page)
  final_count += individual_count
  page += 1
end while individual_count > 20
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1, still ugly and verbose, but in imperative style that's as good as it gets. \$\endgroup\$ – tokland Sep 20 '13 at 8:16
0
\$\begingroup\$

Cryptic but cool way with ruby 2.0 lazy enumerators :

(2..Float::INFINITY).lazy
  .map {|page| get_items(config, page) }
  .take_while {|count| count > 20 }
  .reduce( get_item_count(config, 1), :+ )
# => returns final count

if @toto2 is right and you really mean get_item_count inside the loop :

(1..Float::INFINITY).lazy
  .map {|page| get_item_count(config, page) }
  .take_while {|count| count > 20 }
  .reduce( :+ )
# => returns final count

(I don't have a ruby 2 env at hand now, so didn't try but it should work)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I thought about this, but the OP seems to want the last count greater than 20, so we'd need a take_while that takes the last value. \$\endgroup\$ – tokland Sep 19 '13 at 8:30
  • \$\begingroup\$ oh, did not see that. \$\endgroup\$ – m_x Sep 19 '13 at 8:33
  • \$\begingroup\$ dang. I wish take_while had an :inclusive option. Some guy suggested a take_until that would do that. \$\endgroup\$ – m_x Sep 19 '13 at 8:41
  • \$\begingroup\$ :-) In my answer I was to suggest exactly this, either have an option or a new method. But once you have to monkeypatch the solution loses glamour. I deleted my answer because of this very thing, I couldn't get the last value without making things a lot ugglier. \$\endgroup\$ – tokland Sep 19 '13 at 9:05
0
\$\begingroup\$

Another way to optimize is to keep the individual counts in an array and sum it up at the end.

count = []
until count.any? {|c| c <= 20 }
  count << get_items(config, page)
end
count.reduce(&:+)
# => returns full count

The whole array will be checked at each iteration. But this should not be a performance hit, especially when API communication is involved.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This wouldn't work as the page variable is nowhere incremented. Also, an array of counts would be better named counts not count. \$\endgroup\$ – Rene Saarsoo Sep 30 '13 at 10:20
0
\$\begingroup\$
def final_count(config)
  fc = 0
  (1..1.0/0).each do |page|
    ic = get_item_count(config, page)
    fc += ic
    return fc if ic > 20
  end
end

Uncomfortable with 1.0/0? Then use Float::INFINITY. (Edited to correct error spotted by @Rene. Thanks, @Rene.)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The returned final count will be incorrect, as it doesn't take into account the number of items on the last page. And I'd say this solution is pushing too much towards a one-liner at the cost of readability - for example I find it weird to see a return statement inside the tertiary-operator. \$\endgroup\$ – Rene Saarsoo Sep 30 '13 at 10:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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