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 rule to encourage checking isEmpty over comparing count to zero #206

Merged
merged 1 commit into from
Jan 13, 2016

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Nov 17, 2015

This already helped catch some inefficient calls in SwiftLint 🎉, but I don't think we can merge this as is currently implemented.

I can picture two false positives happening in practice with the way this rule is currently implemented:

  1. Int variable named count. In this case, it'd be fine to check count against zero.
  2. Calling count on types that don't incur a performance hit e.g. struct A { let count: Int }.

I'd appreciate any ideas to help address these two false positives.

@segiddins
Copy link
Contributor

Checking if the type is a collection?

@jpsim
Copy link
Collaborator Author

jpsim commented Nov 17, 2015

Checking if the type is a collection?

How?

@segiddins
Copy link
Contributor

Don't you have the type of the item from the sourcekit request?

@jpsim
Copy link
Collaborator Author

jpsim commented Nov 17, 2015

Don't you have the type of the item from the sourcekit request?

Not quite. You can get that if you have all the required compiler arguments for the given Swift file, which jazzy has but SwiftLint doesn't currently use. To get those, the project has to be fully compiled & indexed and we need to run xcodebuild -dry-run to get the arguments. This is already sub-optimal for jazzy, but completely unacceptable for SwiftLint.

@segiddins
Copy link
Contributor

Ah gotcha, I thought SwiftLint operated on the same source transformation

@jpsim jpsim force-pushed the jp-empty-count-rule branch 2 times, most recently from 874f9e7 to 0adf4fb Compare November 30, 2015 06:00
@jpsim jpsim mentioned this pull request Jan 12, 2016
@jpsim
Copy link
Collaborator Author

jpsim commented Jan 13, 2016

I've revived this now that we have opt-in rules. I'm making this an opt-in rule because it can easily lead to false positives as mentioned in the first comment in this PR. However, this is still tremendously useful as I constantly find myself comparing count against zero.

jpsim added a commit that referenced this pull request Jan 13, 2016
add rule to encourage checking `isEmpty` over comparing `count` to zero
@jpsim jpsim merged commit 3c030e9 into master Jan 13, 2016
@jpsim jpsim deleted the jp-empty-count-rule branch January 13, 2016 23:56
@jpsim jpsim mentioned this pull request Feb 7, 2016
@BluMist
Copy link

BluMist commented Feb 7, 2016

also, with regard to the rule itself...

if I have an optional array,
if array?.count > 0
might be easier to read than
if array?.isEmpty != true
to account for the nil and false scenario

@jpsim
Copy link
Collaborator Author

jpsim commented Feb 7, 2016

This rule isn't so much about style as it is performance. Many data structures allow for more efficient checking if it's empty or not (O(1)) than counting all of the elements (O(n)).

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.

3 participants