Skip to content

Switch .count to .size where appropriate #1605

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

Merged
nimmolo merged 17 commits into from
Sep 11, 2023
Merged

Switch .count to .size where appropriate #1605

Show file tree
Hide file tree
Changes from 6 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.size == 500

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
Copy Markdown
Contributor Author

@nimmolo nimmolo Sep 11, 2023

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
Copy Markdown
Contributor Author

@nimmolo nimmolo Sep 11, 2023

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
Copy Markdown
Member

@pellaea pellaea Sep 11, 2023

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
Copy Markdown
Contributor Author

@nimmolo nimmolo Sep 11, 2023

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.size
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
Copy Markdown
Member

@JoeCohen JoeCohen Sep 11, 2023

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
Copy Markdown
Contributor Author

@nimmolo nimmolo Sep 11, 2023

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

Comment thread
nimmolo marked this conversation as resolved.
Outdated
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
Comment thread
nimmolo marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Member

@JoeCohen JoeCohen Sep 11, 2023

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
Copy Markdown
Member

@JoeCohen JoeCohen Sep 11, 2023

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
Copy Markdown
Contributor Author

@nimmolo nimmolo Sep 11, 2023

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
Comment thread
nimmolo marked this conversation as resolved.
Outdated
@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
Comment thread
nimmolo marked this conversation as resolved.
Outdated
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
Comment thread
nimmolo marked this conversation as resolved.
Outdated
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
Comment thread
nimmolo marked this conversation as resolved.
Outdated
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
Comment thread
nimmolo marked this conversation as resolved.
Outdated
# 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 %>
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<%= 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 %>
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<%= 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 %>
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<%= 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 %>
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<%= 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 %>
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<%= 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 %>:
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<%= 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 %>:
Comment thread
nimmolo marked this conversation as resolved.
Outdated
</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 %>
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<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>
Comment thread
nimmolo marked this conversation as resolved.
Outdated
<% end %>
<%= :pivotal_index_header.tp %>
</div><!--.container-text-->
Expand Down