Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch .count to .size where appropriate #1605

Merged
merged 17 commits into from
Sep 11, 2023
Merged

Switch .count to .size where appropriate #1605

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4a4a7d2
Switch `.count` to `.length` where appropriate
nimmolo Sep 10, 2023
99ffa22
Update _form_list_feedback.html.erb
nimmolo Sep 10, 2023
cefa9bb
Update observation.rb
nimmolo Sep 10, 2023
5acab63
Revert a tested use of `length` -> `size`
nimmolo Sep 11, 2023
b7d3c15
Update app/classes/pivotal.rb
nimmolo Sep 11, 2023
fcee84f
Update app/classes/suggestion.rb
nimmolo Sep 11, 2023
c0e49b3
Update app/controllers/account/preferences_controller.rb
nimmolo Sep 11, 2023
e603681
Update app/controllers/collection_numbers_controller.rb
nimmolo Sep 11, 2023
abeb3ce
Update app/controllers/herbarium_records_controller.rb
nimmolo Sep 11, 2023
22bacd9
Update app/helpers/object_link_helper.rb
nimmolo Sep 11, 2023
85a2b47
Update app/controllers/names/synonyms_controller.rb
nimmolo Sep 11, 2023
8766859
Update app/helpers/object_link_helper.rb
nimmolo Sep 11, 2023
2938902
Update app/presenters/matrix_box_presenter.rb
nimmolo Sep 11, 2023
279ef10
Update app/views/account/preferences/_filters.html.erb
nimmolo Sep 11, 2023
be52bcb
Switch `length` to `size`
nimmolo Sep 11, 2023
1b1e73d
Update high_level_queries.rb
nimmolo Sep 11, 2023
beb53ec
Merge branch 'main' into nimmo-count-to-length
nimmolo Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/classes/pivotal.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def get_stories(_verbose = false)
stories = []

request_stories("", stories)
request_stories("&offset=501", stories) if stories.count == 500
request_stories("&offset=501", stories) if stories.length == 500
nimmolo marked this conversation as resolved.
Show resolved Hide resolved

stories
end
Expand Down
8 changes: 4 additions & 4 deletions app/classes/query/modules/high_level_queries.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module Query::Modules::HighLevelQueries

# Number of results the query returns.
def num_results(_args = {})
@num_results ||= result_ids&.count || 0
@num_results ||= result_ids&.length || 0
end

# Array of all results, just ids.
Expand Down Expand Up @@ -67,7 +67,7 @@ def results(args = {})
# better all be valid instances of +model+ -- no error checking is done!!
def results=(list)
@result_ids = list.map(&:id)
@num_results = list.count
@num_results = list.length
@results = list.each_with_object({}) do |obj, map|
map[obj.id] ||= obj
end
Expand All @@ -78,7 +78,7 @@ def results=(list)
# better all be valid Integer ids -- no error checking is done!!
def result_ids=(list)
@result_ids = list
@num_results = list.count
@num_results = list.length
end

# Get index of a given record / id in the results.
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i now think size may be more appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL


Expand Down
2 changes: 1 addition & 1 deletion app/classes/suggestion.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@JoeCohen JoeCohen Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nimmolo marked this conversation as resolved.
Show resolved Hide resolved
next if count <= best_count

best_name = name
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/account/preferences_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

nimmolo marked this conversation as resolved.
Show resolved Hide resolved
val == "1" ? filter.prefs_vals.first : filter.off_val
else
val.to_s
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/collection_numbers_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@collection_number.observations.first
else
@collection_number
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/herbarium_records_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ def figure_out_where_to_go_back_to
@back_object = Observation.safe_find(@back)
return if @back_object

@back_object = if @herbarium_record.observations.count == 1
@back_object = if @herbarium_record.observations.length == 1
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
@herbarium_record.observations.first
else
@herbarium_record
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/names/synonyms_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def split_off_desynonymized_names(main_name, checks)
pick_one = other_group.shift
pick_one.clear_synonym
other_group.each { |n| pick_one.transfer_synonym(n) }
main_name.clear_synonym if main_name.reload.synonyms.count <= 1
main_name.clear_synonym if main_name.reload.synonyms.length <= 1
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
end

def dump_sorter(sorter)
Expand Down
2 changes: 1 addition & 1 deletion app/helpers/locations_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def country_link(country, count = nil)
# title of a link to Observations at a location, with observation count
# Observations at this Location(nn)
def show_obs_link_title_with_count(location)
"#{:show_location_observations.t} (#{location.observations.count})"
"#{:show_location_observations.t} (#{location.observations.size})"
end

def calc_counts(locations)
Expand Down
4 changes: 2 additions & 2 deletions app/helpers/object_link_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def user_link(user, name = nil, args = {})
def user_list(title, users = [])
return safe_empty unless users&.any?

title = users.count > 1 ? title.to_s.pluralize.to_sym.t : title.t
title = users.length > 1 ? title.to_s.pluralize.to_sym.t : title.t
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
links = users.map { |u| user_link(u, u.legal_name) }
# interpolating would require inefficient #sanitize
# or dangerous #html_safe
Expand Down Expand Up @@ -163,7 +163,7 @@ def description_link(desc)
end

def observation_herbarium_record_link(obs)
count = obs.herbarium_records.count
count = obs.herbarium_records.length
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
if count.positive?

link_to((count == 1 ? :herbarium_record.t : :herbarium_records.t),
Expand Down
1 change: 1 addition & 0 deletions app/models/observation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -971,6 +971,7 @@ def dump_votes
end

# Has anyone proposed a given Name yet for this observation?
# Count is ok here because we have eager-loaded the namings.
def name_been_proposed?(name)
namings.count { |n| n.name == name }.positive?
end
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/matrix_box_presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ def user_to_presenter(user)
# user.contribution and observations.count are just numbers.
self.detail = "#{:list_users_joined.t}: #{user.created_at.web_date}<br/>
#{:list_users_contribution.t}: #{user.contribution}<br/>
#{:Observations.t}: #{user.observations.count}".html_safe
#{:Observations.t}: #{user.observations.length}".html_safe
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
# rubocop:enable Rails/OutputSafety
self.name = user.unique_text_name
self.what = user
Expand Down
4 changes: 2 additions & 2 deletions app/views/account/preferences/_filters.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@

<% ContentFilter.all.each do |filter| %>

<% if filter.type == :boolean && filter.prefs_vals.count == 1 %>
<% if filter.type == :boolean && filter.prefs_vals.length == 1 %>
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<%= render(partial: "account/preferences/filters/checkbox",
locals: { filter: filter, f: f }) %>
<% elsif filter.type == :boolean && filter.prefs_vals.count > 1 %>
<% elsif filter.type == :boolean && filter.prefs_vals.length > 1 %>
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<%= render(partial: "account/preferences/filters/select",
locals: { filter: filter, f: f }) %>
<% elsif filter.type == [:string] %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/collection_numbers/_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ end
<%= form_with(model: @collection_number, url: url_params,
local: local, id: "collection_number_form") do |f| %>

<% if @collection_number.observations.count > 1 %>
<% if @collection_number.observations.length > 1 %>
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<%= alert_block(
:warning, :edit_affects_multiple_observations.t(type: :collection_number)
) %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/herbarium_records/_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ end
<%= form_with(model: @herbarium_record, url: url_params,
local: local, id: "herbarium_record_form") do |f| %>

<% if @herbarium_record.observations.count > 1 %>
<% if @herbarium_record.observations.length > 1 %>
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<%= alert_block(
:warning, :edit_affects_multiple_observations.t(type: :herbarium_record)
) %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/names/synonyms/_fields_existing.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<%# existing synonyms %>

<% if current_synonyms.count > 1 %>
<% if current_synonyms.length > 1 %>
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<%= fields_for(:existing_synonyms) do |fes| %>
<div class="form-group">
<%= content_tag(:div, "#{:form_synonyms_current_synonyms.t}:",
Expand Down
2 changes: 1 addition & 1 deletion app/views/observations/show/_collection_numbers.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ cn_query = Query.lookup(:CollectionNumber, :all, observations: obs.id)
</ul>

<% elsif numbers.any? && !can_edit %>
<%= numbers.count > 1 ? :Collection_numbers.t : :Collection_number.t %>:
<%= numbers.length > 1 ? :Collection_numbers.t : :Collection_number.t %>:
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<%= numbers.map do |number|
link_to("<i>#{number.format_name.t}</i>".html_safe,
collection_number_path(id: number.id, q: cn_query),
Expand Down
2 changes: 1 addition & 1 deletion app/views/observations/show/_herbarium_records.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ query = Query.lookup(:HerbariumRecord, :all, observations: obs.id)

<% elsif records.any? && !can_add %>
<div>
<%= records.count > 1 ? :Herbarium_records.t : :Herbarium_record.t %>:
<%= records.length > 1 ? :Herbarium_records.t : :Herbarium_record.t %>:
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
</div>
<ul class="tight-list">
<% records.each do |record| %>
Expand Down
4 changes: 2 additions & 2 deletions app/views/pivotal/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@
%>

<div class="container-text">
<% if @stories.count > 900 %>
<b>Displaying <%= @stories.count %> of 1,000 available Feature Requests. Please <%= link_to 'let the admins know', '/emails/ask_webmaster_question', title: "Contact Us" %> that we're close to the limit.</b>
<% if @stories.length > 900 %>
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<b>Displaying <%= @stories.length %> of 1,000 available Feature Requests. Please <%= link_to 'let the admins know', '/emails/ask_webmaster_question', title: "Contact Us" %> that we're close to the limit.</b>
nimmolo marked this conversation as resolved.
Show resolved Hide resolved
<% end %>
<%= :pivotal_index_header.tp %>
</div><!--.container-text-->
Expand Down