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/HashLikeCase cop #8280

Merged
merged 1 commit into from Jul 9, 2020
Merged

Conversation

fatkodima
Copy link
Contributor

Closes #8247

when :template then 'template tokens (like `%{foo}`)'
when :unannotated then 'unannotated tokens (like `%s`)'
end
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason you didn't move this to a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are just pieces of the message, not particularly useful for the whole class, so I decided it is cleaner to left it in a method instead of extracting to the top-level constant.
Can extract into a constant, if asked.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll repeat that I feel it's bad style to keep on recreating the same constant hash over and over. Wast of CPU cycles and when reading you have to check the whole hash wondering "Hum, is there anything dynamic in here"?

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 9, 2020

The code looks good overall. You might want to add some tests with an else branch.

@fatkodima
Copy link
Contributor Author

Added a test for else branch.

@bbatsov bbatsov merged commit 72bb99c into rubocop:master Jul 9, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 9, 2020

Thanks!

Description: >-
Checks for places where `case-when` represents a simple 1:1
mapping and can be replaced with a hash lookup.
Enabled: 'pending'
Copy link
Member

Choose a reason for hiding this comment

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

Will this cop be enabled by default at RuboCop 1.0? I'm not sure about creating a constant of hash instance is a better style than case syntax if there is no performance advantage. I think it may be considered disabled by default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it most cases using the hash results in code that's simpler, as you eliminate branching completely. Obviously a simple case is not a big problem, but I feel that in most cases using a hash results in cleaner code.

@Drenmi
Copy link
Collaborator

Drenmi commented Jul 23, 2020

Technically a case with an else branch can be replaced with a hash with a default value, but it's not nearly as neat as a hash literal, so I think this is a good cut-off. 🙂

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.

New cop to suggest replacing case/when with hash lookup
5 participants