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

Implement a cop that checks for something.length == 0 #2620

Closed
bbatsov opened this issue Jan 10, 2016 · 11 comments
Closed

Implement a cop that checks for something.length == 0 #2620

bbatsov opened this issue Jan 10, 2016 · 11 comments

Comments

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 10, 2016

We should suggest replacing such checks with empty?.

This cop should also handle != and the common alias size.

@jawshooah
Copy link
Contributor

Is coll.any? equivalent to !coll.empty? in terms of performance? If so, the check should probably suggest it as a replacement for coll.length != 0.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jan 12, 2016

It's not the same in terms of semantics.

@jawshooah
Copy link
Contributor

Really? What's the difference? (I ask because I've been using #any? as an equivalent to !#empty? and really hope I haven't been introducing subtle bugs in my code)

@jawshooah
Copy link
Contributor

I know #any? takes an optional predicate block, but without the block it just returns true on the first element it sees (I would hope). Found the difference. Excuse my ignorance!

@rrosenblum
Copy link
Contributor

Interesting. I am probably guilty of using any? as a replacement to !#empty?. Glad to have learned something here.

@alexdowad
Copy link
Contributor

Another difference.

5:50 ~ % pry
[1] pry(main)> "".any?
NoMethodError: undefined method `any?' for "":String
from (pry):1:in `__pry__'
[2] pry(main)> "".empty?
=> true

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 14, 2016

The same check should probably also be done for the logically equivalent:

0 == something.length

This might seem weird to some, but the habit is carried over by some from languages where this has benefit.

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jan 14, 2016

Yeah, we should check this this as well.

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 19, 2016

I implemented this cop as an exercise, and soon realized that something can be whatever the user wants, as long as it implements length or size. But that something may not necessarily also implement empty?, as is the case, for example, with Parser::Source::Range. (Causing two offenses in the Rubocop source.)

What is the official strategy for dealing with such user implemented "duds"? I can see three possible paths, none of which seem ideal to me:

  1. Ignore it and let the user mark it as a false positive. (This could make a potential --auto-correct rather dangerous.)
  2. Check if the receiver is a known implementor of empty?. (This could get rather verbose.)
  3. Do something funky and check if the receiver respond_to?(:empty?). (This isn't really static code analysis any more, is it?)

Please enlighten me. 😃

@bbatsov
Copy link
Collaborator Author

bbatsov commented Jan 19, 2016

Ignore it and let the user mark it as a false positive. (This could make a potential --auto-correct rather dangerous.)

This is the answer. :-)

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 31, 2016

Added in #2683. I guess this issue can be closed now.

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

No branches or pull requests

5 participants