You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/01/18 01:18:53 UTC
[GitHub] [incubator-hudi] lamber-ken opened a new pull request #1245:
[MINOR] Replace Collection.size > 0 with Collection.isEmpty()
lamber-ken opened a new pull request #1245: [MINOR] Replace Collection.size > 0 with Collection.isEmpty()
URL: https://github.com/apache/incubator-hudi/pull/1245
## What is the purpose of the pull request
Use isEmpty() to check whether the collection is empty or not.
## Brief change log
- *Use isEmpty() to check whether the collection is empty or not.*
## Verify this pull request
This pull request is code cleanup without any test coverage.
## Committer checklist
- [ ] Has a corresponding JIRA in PR title & commit
- [ ] Commit message is descriptive of the change
- [ ] CI is green
- [ ] Necessary doc changes done or have another open PR
- [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] lamber-ken commented on issue #1245: [MINOR]
Replace Collection.size > 0 with Collection.isEmpty()
Posted by GitBox <gi...@apache.org>.
lamber-ken commented on issue #1245: [MINOR] Replace Collection.size > 0 with Collection.isEmpty()
URL: https://github.com/apache/incubator-hudi/pull/1245#issuecomment-576026737
hi @vinothchandar @smarthi thanks for review this pr.
Here are my thoughts:
1, It's right that some collections use `size()==0` inside their `isEmpty()` method, but that doesn't mean that they all do, for example `java.util.concurrent.ConcurrentSkipListSet`.
The [ConcurrentSkipListSet documentation](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentSkipListSet.html) says:
```
Beware that, unlike in most collections, the size method is not a constant-time operation.
```
It means that `.size()` can be O(1) or O(N), depending on the data structure; `.isEmpty()` is never O(N).
<br>
2, IDE always prompts me to make changes.
<br>
3, In hudi project, some codes use `isEmpty()`, some codes `size() > 0`
<br>
4, `isEmpty()` is a clearer definition of what it is we actually care about, IMO, it's more understandable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] smarthi commented on issue #1245: [MINOR] Replace
Collection.size > 0 with Collection.isEmpty()
Posted by GitBox <gi...@apache.org>.
smarthi commented on issue #1245: [MINOR] Replace Collection.size > 0 with Collection.isEmpty()
URL: https://github.com/apache/incubator-hudi/pull/1245#issuecomment-575941333
> I am not sure if this is really improving things. its very subjective. I'd argue reading `!isEmpty()` is harder than a simple positive check..
>
> Not sure if I want to really make this change
I agree, maybe consider rebasing this PR after PR# 1159 is merged after 0.5.1 release - PR# 1159 introduces CollectionUtils - u could add this as a method there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] lamber-ken edited a comment on issue #1245:
[MINOR] Replace Collection.size > 0 with Collection.isEmpty()
Posted by GitBox <gi...@apache.org>.
lamber-ken edited a comment on issue #1245: [MINOR] Replace Collection.size > 0 with Collection.isEmpty()
URL: https://github.com/apache/incubator-hudi/pull/1245#issuecomment-576026737
hi @vinothchandar @smarthi thanks for review this pr.
Here are my thoughts:
1, It's right that most collections use `size()==0` inside their `isEmpty()` method, but that doesn't mean that they all do, for example `java.util.concurrent.ConcurrentSkipListSet`.
The [ConcurrentSkipListSet documentation](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentSkipListSet.html) says:
```
Beware that, unlike in most collections, the size method is not a constant-time operation.
```
It means that `.size()` can be O(1) or O(N), depending on the data structure; `.isEmpty()` is never O(N).
<br>
2, IDE always prompts me to make changes.
<br>
3, In hudi project, some codes use `isEmpty()`, some codes `size() > 0`
<br>
4, `isEmpty()` is a clearer definition of what it is we actually care about, IMO, it's more understandable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-hudi] lamber-ken closed pull request #1245: [MINOR]
Replace Collection.size > 0 with Collection.isEmpty()
Posted by GitBox <gi...@apache.org>.
lamber-ken closed pull request #1245: [MINOR] Replace Collection.size > 0 with Collection.isEmpty()
URL: https://github.com/apache/incubator-hudi/pull/1245
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services