Add Rails/FindByOrAssignmentMemoization cop

[r7kamura]

It is common to see code that attempts to memoize find_by result by ||=, but find_by may return nil, in which case it is not memoized as intended.

def current_user
  @current_user ||= User.find_by(id: session[:user_id])
end

In most cases, a query cache is used, but even so, performance degradation due to unnecessary processing and object creation can still occur.

See also:

• Add find-by-memoization section rails-style-guide#358

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[Earlopain]

I like this, its a common pitfall I've encountered many times myself and is most likely not what the programmer intended.

Let's add this as a test (works fine already, no offense):

@current_user ||= User.find_by(id: session[:user_id]) || User.anon

@current_user ||= session[:user_id] ? User.find_by(id: session[:user_id]) : nil

and this pattern (also fine, correct autocorrect):

@current_user ||= User.find_by(id: session[:user_id]) unless session[:user_id].nil?

---

For what I presume would be the most common case, I'd write the correction as follows:

def current_user
  @current_user ||= User.find_by(id: session[:user_id])
end

# =>

def current_user
  return @current_user if instance_variable_defined?(:@current_user)

  @current_user = User.find_by(id: session[:user_id])
end

I just find it easier to parse than the if/else assignment to a single variable, but that only really works when the memoization is the only line of code, so eh.

---

I would exclude controllers from being inspected by this cop. They execute once and don't benefit from changing the code.. Well, actions wouldn't but those aren't the only methods a controller can have. I guess my thought here doesn't really work out.

[koic]

If this is a new cop, consider making a corresponding update to the Rails Style Guide.

Can you open to the Rails Style Guide first?

[r7kamura]

Thanks, I just proposed a pull request to rubocop/rails-style-guide:

• Add find-by-memoization section rails-style-guide#358