-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add suggestions for checking if collection is empty #371
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.
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
base: master
Are you sure you want to change the base?
Conversation
README.adoc
Outdated
@@ -1069,6 +1069,25 @@ User.all.size | |||
User.all.length | |||
---- | |||
|
|||
=== Check if the collection is empty [[check-empty-collection]] | |||
|
|||
When checking if the unloaded Active Record collection is empty, prefer `any?`/`empty?` over `present?`/`blank?`. The former executes a simple `EXISTS`-like query while the latter loads the whole collection to determine it's size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When checking if the unloaded Active Record collection is empty, prefer `any?`/`empty?` over `present?`/`blank?`. The former executes a simple `EXISTS`-like query while the latter loads the whole collection to determine it's size. | |
When checking if the unloaded Active Record collection is empty, prefer `any?`/`empty?` over `present?`/`blank?`. The former executes a simple `EXISTS`-like query while the latter loads the whole collection to determine its size. |
ea8d186
to
9a1323a
Compare
@@ -1069,6 +1069,25 @@ User.all.size | |||
User.all.length | |||
---- | |||
|
|||
=== Check if the collection is empty [[check-empty-collection]] | |||
|
|||
When checking if the unloaded Active Record collection is empty, prefer `any?`/`empty?` over `present?`/`blank?`. The former executes a simple `EXISTS`-like query while the latter loads the whole collection to determine its size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is even more descriptive.
user.articles.any?
is used just as frequently as users.where(...).any?
, and in the former case, any?
behavior is not equivalent to exists?
, and is not behaving similarly to empty?
.
users.where(active: true).empty? | ||
|
||
# good - the collection is used after loading | ||
users.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced that preloading the list of records implicitly with present?
is a practice we should recommend to everyone.
@@ -1069,6 +1069,25 @@ User.all.size | |||
User.all.length | |||
---- | |||
|
|||
=== Check if the collection is empty [[check-empty-collection]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generic. The guideline is specific to "not loaded collection".
It is hard to know if it's loaded or not if it e.g. comes as an argument to a method, or elsewhere:
def highlight?
recent_articles.any?
end
users.where(active: true).empty? | ||
|
||
# good - the collection is used after loading | ||
users.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there are hundreds of thousands of those records?
users.where(active: true).any? | ||
users.where(active: true).empty? | ||
|
||
# good - the collection is used after loading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am referring to the "used" wording above.
Even though the collection will be used a few lines below, maybe only a fraction of it will be actually used with a batching iterator like `users.find_each { ...?
Or it will be further queried upon e.g. users.active
? Or users.last(3)
.
Or, it will be used in a sub-query, and it doesn't meet the purpose to load them all e.g. Articles.recent.joins(:author).merge(users)
.
|
||
# good - the collection is used after loading | ||
users.present? | ||
users.each(&:notify) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pirj If you have suggestions for better wording/examples, I think it is better to force push into the PR. I used simple examples (not caring about |
Reference rubocop/rubocop-rails#1529