Switch `.count` to `.size` where appropriate #1605

nimmolo · 2023-09-10T20:52:11Z

Should resolve a number of mysterious n+1's.

Discussion in #1598, starting here

Please check the changes thoroughly, especially to classes and the model.

coveralls · 2023-09-10T21:15:07Z

coverage: 93.642%. remained the same when pulling 1b1e73d on nimmo-count-to-length into 5bd1b1e on main.

nimmolo · 2023-09-10T21:22:56Z

There's a count in shared/_form_list_feedback.rb that if changed, exposes the need for whichever calling action to eager-load observations.

But many controllers may call this partial, and hmmm.... where to add the includes. It only occurs when there's a problem with the name in the form. Not a big deal in any case. Moving on...

JoeCohen · 2023-09-10T23:56:23Z

app/classes/suggestion.rb

@@ -68,7 +68,7 @@ def name_with_most_observations(names)
    best_name = nil
    best_count = -1
    names.each do |name|
-      count = name.observations.count
+      count = name.observations.length


I don't know if length is right; I don't know whether this is right. (I don't how where it gets used in user workflow.)
Maybe use size here and in other places? Is that bad form?

@JoeCohen I don't know either, but I can answer one part of it. According to internet, size is an alias of length. (on Medium)(on Ruby in Rails).

Update: Sorry, size is NOT an alias of length. Thank you Stack Overflow!

However, that's an old answer. In the console, I notice that length on an association that has already been fetched only counts the number of cached records, so that may have changed.

After reading through that whole thread, i think I should go with size. Thanks for figuring that out.

You made me look.
If we need the records, length is better.
Else size is better.

length(). Link
Returns the size of the collection calling size on the target. If the collection has been already loaded, length and size are equivalent. If not and you are going to need the records anyway this method will take one less query. Otherwise size is more efficient.

So it looks like size is faster here. But I don't know if it's faster enough to be worth worrying about.

From Roy Lindauer
count on a collection will always execute a COUNT() SQL statement to get an accurate count value. size returns the size of the collection, but will execute a COUNT() SQL statement if it's not already loaded. length returns the size of the collection from memory. If the collection has already been loaded and is in memory length and size are equivalent.

Relying on what's in memory may give you inaccurate results as the database could be modified while you are working on that collection. But making a bunch of expensive database calls to get an accurate count is probably something you want to avoid. There may be times when a count is preferred, but when in doubt it's probably better to use length.

also see diagrams here: https://www.akshaykhot.com/rails-length-size-count-difference/

I'm going to post all this stuff in a GitHub Discussion for reference.

JoeCohen · 2023-09-11T00:18:10Z

app/controllers/account/preferences_controller.rb

@@ -100,7 +100,7 @@ def update_pref(pref, val)
    def update_content_filter(pref, val)
      filter = ContentFilter.find(pref)
      @user.content_filter[pref] =
-        if filter.type == :boolean && filter.prefs_vals.count == 1
+        if filter.type == :boolean && filter.prefs_vals.length == 1


Though it's not the point of this PR, can you use
one?
instead of
length == 1

Yes! I don't know that method. Will alter

app/helpers/locations_helper.rb

nimmolo · 2023-09-11T02:06:57Z

app/classes/query/modules/high_level_queries.rb

@@ -112,7 +112,7 @@ def paginate_ids(paginator)
        ids = ids.select { |id| @letters[id] == letter }
      end
    end
-    paginator.num_total = ids.count
+    paginator.num_total = ids.length
    ids[paginator.from..paginator.to] || []
  end


@pellaea is this OK in Query::HighLevelQueries? At the point of figuring out the paginator and the @num_results, the query is done, so it seems length is more appropriate.

It may make no difference currently, but it will probably matter if Query starts using scopes.

Ok i now think size may be more appropriate.

Oh dear god! I can't possibly read that entire email thread right now!

I do want to make sure one thing is pointed out in this one case, though -- ids here is not a query, it's just a plain old Array. So there's no question of it executing SQL queries or anything if "done wrong".

(Is there in fact any difference between size, count and length on a plain old Array??... oh hold on, strike that question... it's almost certainly answered somewhere in the email thread I just deleted! :)

app/classes/pivotal.rb

app/classes/suggestion.rb

Switch to `size`

app/controllers/account/preferences_controller.rb

Use `one?`

app/controllers/collection_numbers_controller.rb

Switch to `one?`

JoeCohen · 2023-09-11T02:32:15Z

app/controllers/collection_numbers_controller.rb

@@ -350,7 +350,7 @@ def figure_out_where_to_go_back_to
      @back_object = Observation.safe_find(@back)
      return if @back_object

-      @back_object = if @collection_number.observations.count == 1
+      @back_object = if @collection_number.observations.length == 1


Another place for one?, which is defined as

# File activerecord/lib/active_record/relation.rb, line 282 def one? return super if block_given? limit_value ? records.one? : size == 1 end

https://github.com/rails/rails/blob/56bcc0abd3c9a6b09469e9428f6eea0dd77c2294/activerecord/lib/active_record/relation.rb#L282

Note: I'm fine with not using one? here and elsewhere. I'm just putting it out as an option.

Oops. Probably no on size, stick with length, as observations is used on the next line.

OK. I went with one? - If you get a chance please have another look over everything — i've changed everything in the PR — and check if it seems right.

app/controllers/herbarium_records_controller.rb

switch to `one?`

app/helpers/object_link_helper.rb

Switch to `size`

app/controllers/names/synonyms_controller.rb

Switch to `size`

app/helpers/object_link_helper.rb

Switch to `size`

app/presenters/matrix_box_presenter.rb

Switch to `size`

app/views/account/preferences/_filters.html.erb

Switch to `one?`

app/views/account/preferences/_filters.html.erb

app/views/collection_numbers/_form.html.erb

app/views/herbarium_records/_form.html.erb

app/views/names/synonyms/_fields_existing.html.erb

app/views/observations/show/_collection_numbers.html.erb

app/views/observations/show/_herbarium_records.html.erb

app/views/pivotal/index.html.erb

Switch to `size`

JoeCohen · 2023-09-11T02:56:49Z

Thank you. A nice, succinct explanation of the differences, plus a link to a very useful flow chart.

…

On Sun, Sep 10, 2023 at 7:23 PM andrew nimmo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/classes/suggestion.rb <#1605 (comment)> : > @@ -68,7 +68,7 @@ def name_with_most_observations(names) best_name = nil best_count = -1 names.each do |name| - count = name.observations.count + count = name.observations.length From Roy Lindauer <https://www.roylindauer.com/blog/the-difference-between-length-size-and-count-in-ruby-on-rails.html> count on a collection will always execute a COUNT(*) SQL statement to get an accurate count value. size returns the size of the collection, but will execute a COUNT(*) SQL statement if it's not already loaded. length returns the size of the collection from memory. If the collection has already been loaded and is in memory length and size are equivalent. Relying on what's in memory may give you inaccurate results as the database could be modified while you are working on that collection. But making a bunch of expensive database calls to get an accurate count is probably something you want to avoid. There may be times when a count is preferred, but when in doubt it's probably better to use length. also see diagrams here: https://www.akshaykhot.com/rails-length-size-count-difference/ I'm going to post all this stuff in a GitHub Discussion for reference. — Reply to this email directly, view it on GitHub <#1605 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALDFHTHQZY7YY4ICBVHCTXZZYZVANCNFSM6AAAAAA4SND3XQ> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

JoeCohen · 2023-09-11T03:00:50Z

I'm out of time and probably won't be able to return to this PR.
One note of caution: one?, many?, and none? all call size.
So length == 1 etc. may be preferable to one? etc. in cases where length is preferable to size.

Switch .count to .length where appropriate

4a4a7d2

nimmolo requested review from JoeCohen, mo-nathan and pellaea September 10, 2023 20:52

Update _form_list_feedback.html.erb

99ffa22

nimmolo marked this pull request as ready for review September 10, 2023 21:15

Update observation.rb

cefa9bb

JoeCohen reviewed Sep 10, 2023

View reviewed changes

JoeCohen reviewed Sep 11, 2023

View reviewed changes

app/helpers/locations_helper.rb Outdated Show resolved Hide resolved

nimmolo added the <= 20 files label Sep 11, 2023