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.

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

Add new Style/CombinableLoops cop #8486

Merged
merged 1 commit into from Aug 24, 2020

Conversation

fatkodima
Copy link
Contributor

This cop checks for places where multiple consecutive loops over the same data can be combined into a single loop.
It is very likely that combining them will make the code more efficient and more concise.

  # bad
  def method
    items.each do |item|
      do_something(item)
    end

    items.each do |item|
      do_something_else(item)
    end
  end

  # good
  def method
    items.each do |item|
      do_something(item)
      do_something_else(item)
    end
  end

Inspired by https://rules.sonarsource.com/java/RSPEC-3047

I ran it on ~30k files and it detected 133 offenses. So seems useful.

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Interesting. You didn't want to raise on offense for combinable each with each_with_index?

lib/rubocop/cop/style/combinable_loops.rb Outdated Show resolved Hide resolved
config/default.yml Show resolved Hide resolved
spec/rubocop/cop/style/combinable_loops_spec.rb Outdated Show resolved Hide resolved
@@ -15,18 +15,14 @@
a %{op} b
RUBY
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I love it when you find things in this codebase 👍

@fatkodima
Copy link
Contributor Author

Updated with suggestions.


def collection_looping_method?(node)
method_name = node.send_node.method_name
method_name.match?(/^each/) || method_name.match?(/_each$/)
Copy link
Contributor

Choose a reason for hiding this comment

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

the || method_name.match?(/_each$/) is still not covered by the specs, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test for reverse_each

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

LGTM. I'm usggesting a small wording change, and maybe @bbatsov has a better idea for the cop's name?

lib/rubocop/cop/style/combinable_loops.rb Outdated Show resolved Hide resolved
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 24, 2020

I'm guessing we should make this work with for as well, right? As for the name - I can't really think of a better name right now. I'm thinking, however, it'd be nice to have a similar check for conditionals - e.g. consecutive ifs with the same condition.

@fatkodima
Copy link
Contributor Author

I'm guessing we should make this work with for as well, right?

Yes, added support for it.

I'm thinking, however, it'd be nice to have a similar check for conditionals - e.g. consecutive ifs with the same condition.

I guess it will be better implemented as a new cop?

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 24, 2020

I guess it will be better implemented as a new cop?

Yeah, definitely.

@bbatsov bbatsov merged commit 112ae91 into rubocop:master Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants