CollectionAssociation shadows Enumerable#count

In the example in the issue, wouldn't this work nearly as well:

def incomplete_submissions
  submissions.to_a.count(&:incomplete?)
end

In short, if you want the Enumerable behavior, just ask for it...

--Matt Jones

I just ran into this issue, and I would actually call it a bug since it causes very unexpected behavior:

> User.count { |user| user.not_a_method! }
=> 10

So the block isn’t even being evaluated. I’m fine if we don’t delegate to Enumerable, but I would expect #count to at least raise an error if a block is given so people don’t accidentally try to count with a predicate and get the wrong answer.

Agreed :+1:

Also agree with Matt Wean and Miles on this one.

I’ve never passed an argument to a .count method in my 9 years of writing ruby code. I would never think to do such a thing and such a thing would never occur to me (passing anything — block or otherwise — to a count method)

Here’s the efficiency problem with your code & proposal:

submissions.to_a.count(&:incomplete?)

This code inherently encourages a code smell design— the objects must first be retrieved from the database before they are evaluated for incomplete?. This is slow, and won’t scale.

Instead, you would do well to define incomplete as a scope and not a method and instead rewrite it like so:

submissions.incomplete.count

That will correctly use Active-Relation (A-Rel) objects to create the right SQL, something like

SELECT count(*) FROM submissions where incomplete = 1

Otherwise, you fetch all submissions from the database and then call incomplete? on each one, creating both an N+1 query problem and also a scaling problem because you have to instantiate each object (object instantiation is not fast) just to find out if it responds true or false.

For efficiency alone, I would be :thumbs_down: on a change like this as it would encourage this kind of bad coding practices.

-Jason

I think this was merged into 5.1 (https://github.com/rails/rails/pull/24203).

  • Phil